Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use .d.ts instead of .vue.d.ts for Vue declarations #336

Merged
merged 2 commits into from Jun 1, 2022

Conversation

agilgur5
Copy link
Collaborator

Summary

For the rollup-plugin-vue integration, output declarations as name.d.ts instead of name.vue.d.ts

Details

ezolenko and others added 2 commits May 31, 2022 18:21
- `.vue.d.ts` was recommended by the `rollup-plugin-vue` maintainers
  back when this code was originally written, but apparently it causes
  problems with some imports
  - plain `.d.ts` seems to work fine according to users
    - and plain is the standard in TS too I believe
@agilgur5 agilgur5 added kind: bug Something isn't working properly scope: integration Related to an integration, not necessarily to core (but could influence core) labels May 31, 2022
@ezolenko ezolenko merged commit e145d0b into ezolenko:master Jun 1, 2022
@agilgur5 agilgur5 assigned agilgur5 and unassigned agilgur5 Jun 2, 2022
@andrew0
Copy link

andrew0 commented Jul 9, 2022

This change is breaking Vue type definitions for me. I put up a small repro here: https://github.com/andrew0/rollup-vue-ts-repro. The index.d.ts file contains

export { default as MyComponent } from './MyComponent.vue';

so the component's type definition should be MyComponent.vue.d.ts, not MyComponent.d.ts. I'm not really sure why the people in the original issue were having problems with the old behavior. Perhaps they were importing .vue files without an extension, but this was never recommended, and is no longer supported by the Vue team.

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 20, 2022

I'm not really sure why the people in the original issue were having problems with the old behavior.

@andrew0 this isn't the original issue, so I think it would be best to discuss that there.
I'm not a Vue user (and I don't believe ezolenko is either), so we've just been going with what users have asked us to implement. I count at least 4 people in that issue between comments and reactions (and most users do not interact with issues at all), so a non-trivial amount had asked for this.

Perhaps they were importing .vue files without an extension, but this was never recommended, and is no longer supported by the Vue team.

It's possible, but there isn't a repro there to work off of and deduce. rollup-plugin-vue isn't supported either so this integration can be considered deprecated too. Not sure if Vue 2 / Vue 3 makes a difference either?

I see that issue got a decent amount of references (and probably has many more if it were not locked), so it sounds very possible that extensionless was pretty commonly used by a variety of users and was even supported by the CLI. It also mentions configuring Webpack for resolving the extension.
The original issue predates that Vite issue too
TS itself also commonly uses extensionless (Node ESM breaks that a bit though), so I can see this being pretty confusing to the community; whether or not that is the reasoning behind the original issue.

next steps

I've dug into Vue a bit more in supporting rpt2; I think we'd probably want to follow whatever vue-tsc does ideally to follow "principle of least surprise" as we try to do with tsc already.
We'd also want to make sure the type definitions work with Volar -- which should hopefully function the same.

@andrew0 I think it would still be good for you to comment on the original issue to see what the users there think.

I put up a small repro here: https://github.com/andrew0/rollup-vue-ts-repro.

There's another use-case that users might have been talking about too, importing Vue SFCs from each other. e.g. if you had MyComponent2 imported by MyComponent, what would vue-tsc output and how would Volar interact with type definitions there?
Ideally the compiled files should work without a Vue-specific tool like Volar etc, so we'd want to check compat on that too. And make sure things like auto-complete etc work. Lot of things to double-check if we want certainty 😕

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 23, 2022

I put up a small repro here: https://github.com/andrew0/rollup-vue-ts-repro.

I added vue-tsc and some additional config to your repro and confirmed that vue-tsc produces .vue.d.ts files, and not .d.ts files.

I went ahead and created a Vue repro environment here (forked from the original rpt2-repro environment) for use for this and other Vue issues. That has updated config, uses Vue 3, etc, and similarly has .vue.d.ts from vue-tsc.

Based on this comparison, it seems like we may very well want to revert this PR in the next breaking release (0.33.0).

@andrew0 commenting on the original issue and getting clarification there would still be very helpful. Don't want to churn this more than it has already, and not sure if users there were using a common misconfiguration (well, now it is) or what

Not sure if Vue 2 / Vue 3 makes a difference either?

It looked like you were using Vue 2 in your repro, though I had the same results with Vue 3 in my repro. Not sure if this might be because of how vue-tsc is set-up though.

I haven't yet checked in on Volar and other imports etc (mostly as I'm not a Vue user).

@andrew0
Copy link

andrew0 commented Jul 24, 2022

Thanks a lot for looking into this more, @agilgur5! I agree it makes sense to align with what vue-tsc does. I left a comment in the original issue to try to clarify why the filer was having issues with the old behavior.

There's another use-case that users might have been talking about too, importing Vue SFCs from each other. e.g. if you had MyComponent2 imported by MyComponent, what would vue-tsc output and how would Volar interact with type definitions there?

I modified your repro a bit to test how vue-tsc would handle this: https://stackblitz.com/edit/rpt2-vue-repro-2jy8kh. I think in most cases, importing .vue files from another .vue file works the same as importing a .vue file from a .ts file. One difference I did notice is that it seems that vue-tsc is able to merge the prop types when you extend components, whereas rpt2 does not.

vue-tsc:

declare const _default: import("vue").DefineComponent<{
    b: StringConstructor;
}, unknown, unknown, {}, {}, import("vue").ComponentOptionsMixin, import("vue").DefineComponent<{
    a: StringConstructor;
}, unknown, unknown, {}, {}, import("vue").ComponentOptionsMixin, import("vue").ComponentOptionsMixin, Record<string, any>, string, import("vue").VNodeProps & import("vue").AllowedComponentProps & import("vue").ComponentCustomProps, Readonly<import("vue").ExtractPropTypes<{
    a: StringConstructor;
}>>, {}>, Record<string, any>, string, import("vue").VNodeProps & import("vue").AllowedComponentProps & import("vue").ComponentCustomProps, Readonly<import("vue").ExtractPropTypes<{
    b: StringConstructor;
}>>, {}>;
export default _default;

rollup-plugin-typescript2:

declare const _default: import("vue").DefineComponent<{
    b: StringConstructor;
}, unknown, unknown, {}, {}, import("vue").ComponentOptionsMixin, any, Record<string, any>, string, import("vue").VNodeProps & import("vue").AllowedComponentProps & import("vue").ComponentCustomProps, Readonly<import("vue").ExtractPropTypes<{
    b: StringConstructor;
}>>, {}>;
export default _default;

So with vue-tsc, Volar would be able to give autocomplete suggestions for both the a and b props, whereas with rpt2, Volar would only see the b prop.

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 29, 2022

Thanks a lot for looking into this more, @agilgur5!

Thanks for engaging @andrew0 🙂
We could definitely use help on the Vue front with this issue and a few others! If you have any time to look through the open Vue issues here (there's only 2 I believe), that could really help given our lack of Vue knowledge 🙁

So with vue-tsc, Volar would be able to give autocomplete suggestions for both the a and b props, whereas with rpt2, Volar would only see the b prop.

To clarify, Volar only picks this up with the .vue.d.ts extension, right?
As in, with rpt2 0.32's .d.ts only, Volar doesn't work? But it works works with rpt2 <0.32?

Or does it work with both?

One difference I did notice is that it seems that vue-tsc is able to merge the prop types when you extend components, whereas rpt2 does not.

Thanks for the example! I believe this illustrates an existing (and fairly complicated) issue, #129.
The difference here being that the shims-vue.d.ts file basically gives a "default" .vue type declaration, so TS doesn't completely fail to resolve the type as a result.

If I had to guess, vue-tsc is aware that files it processes are only TS or Vue, and therefore it can do Vue -> TS -> DTS or something (would need to look at the vue-tsc source more closely, given that the docs are fairly sparse).
Per #129, the difficulty with Rollup is that there can be any number of plugins on-chain, so rpt2 can't just delegate to a single one. That and that TS has a synchronous interface, whereas Rollup has an async one (would need to use something like deasync to support Rollup's this.resolve / this.load inside of the TS LanguageServer).
TS needs to have knowledge of the whole "Program"; it can't just compile per-file in order to do semantic type-checking or to produce declarations. So basically, when it sees a .vue file in the import chain, it needs to be able to resolve the types for that file as it may impact the file currently being processed. But rpt2 itself doesn't know what to do with .vue SFC files or how to otherwise get their types, causing #129.
Note that the only current integration with rpv is the one-line in this PR, which is a one-way integration. Per #129, there's some circular integration required to support this, rpv -> rpt2 -> rpv -> rpt2 etc.

@agilgur5 agilgur5 added kind: regression Specific type of bug -- past behavior that worked is now broken solution: reverted This change was reverted after being determined to contain a regression labels Aug 18, 2022
@agilgur5
Copy link
Collaborator Author

Reverted in #410 and released in 0.33.0

@agilgur5 agilgur5 added the scope: vue Related to integration with Vue (rollup-plugin-vue is long archived), not core label Mar 14, 2023
@agilgur5 agilgur5 deleted the fix-vue-virtual-filename-parsing branch July 2, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly kind: regression Specific type of bug -- past behavior that worked is now broken scope: integration Related to an integration, not necessarily to core (but could influence core) scope: vue Related to integration with Vue (rollup-plugin-vue is long archived), not core solution: reverted This change was reverted after being determined to contain a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vue declaration files endings -- .vue.d.ts vs. .d.ts
3 participants