fix: type augmentation and compiler-sfc types w/moduleResolution: bundler (fix #13106) #13107
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When using typescript's "moduleResolution": "bundler" option, we currently get a bunch of type errors, particularly when trying to extend the types, or when consuming other packages which extend the types (vue-router, pinia, etc).
For example, vue-router extends the types as follows:
```ts
declare module 'vue/types/vue' {
interface Vue {
$router: VueRouter
$route: Route
}
}
declare module 'vue/types/options' {
interface ComponentOptions<V extends Vue> {
router?: VueRouter
beforeRouteEnter?: NavigationGuard<V>
beforeRouteLeave?: NavigationGuard<V>
beforeRouteUpdate?: NavigationGuard<V>
}
}
```
But this silently fails (when using skipLibChecks: true), and the types aren't available on the vue constructor options or the component instance.
The reason for this can be seen when copying the code into your own project with `"moduleResolution": "bundler"`:
```
Invalid module name in augmentation, module 'vue/types/vue' cannot be found.ts(2664)
module "vue/types/vue"
```
What is happening is that once "moduleResolution" is set to "bundler", the "exports" maps in package.json starts being used, and there are some peculiarities with wildcard matching that we don't appear to be accounting for here. In particular, wildcard matching is going to require that the full path to the file is provided. So when we specify `declare module 'vue/types/vue'` it is going to look for a file on disk with the relative path: `./types/vue`, and since there is no file with that name, it fails.
What this PR does is provide multiple options to look for when an import matches the "./types/*" export.
1) First we append '.d.ts' and check for that file. I suspect this will be the most common occurence (people that didn't use the extension) so I'm listing it first.
2) In the event that someone had done `declare module 'vue/types/vue.js'` or `declare module 'vue/types/vue.d.ts'` in their code to work around this issue in prior versions of vue 2.7.x, we still allow for those to match directly.
Fixes: vuejs#13106
Types were broken when consuming the "./compiler-sfc" subpath export from an esm context. To reproduce, you can set `"type": "module"` in package.json, and "moduleResolution": "bundler" or "node16" in your tsconfig.json. This still works by accident for `"type": "commonjs"` because when it matches `"require": "./compiler-sfc/index.js"` typescript will also check for the corresponding `index.d.ts` file. However, for `"./compiler-sfc/index.mjs"` it winds up checking for `index.d.mts` and doesn't find one. After this commit, types should be working in all cases for the compiler-sfc entrypoint with moduleResolution: "node", "node16", and "bundler" when the consuming package is listed as "type": "module" or "commonjs".
This change is somewhat optional as things are working as currently specified, but the typescript team suggests in their documenation that the "types" condition should be listed first to guarantee that its used. The main difference is that when "types" is listed first, it will match early in the process and just use the types that you specified. If it is listed last, typescript will first match on the "require", "import", "default" conditions that appear higher up in the list. It then does extension modification to try to find corresponding .d.ts files. In more concrete terms, typescript will wind up looking for "./dist/vue.runtime.esm.d.ts" or "./dist/vue.runtime.common.d.ts" (depending on context) first. Since these files don't exist, it will skip those conditions and keep going until it ultimately finds the "types" entry and use that. After moving "types" to be the first condition, we get the same result, but skip the extra checks.
thebanjomatic
commented
Oct 24, 2023
| "import": "./compiler-sfc/index.mjs", | ||
| "require": "./compiler-sfc/index.js" | ||
| }, | ||
| "./dist/*": "./dist/*", | ||
| "./types/*": "./types/*", | ||
| "./types/*": ["./types/*.d.ts", "./types/*"], |
There was a problem hiding this comment.
To help clarify the conversion from import name to file lookups, I tried to follow the pseudo-code from the documentation here and list out what would happen in the following scenarios:
Before this change:
"vue/types/options" => ["./types/options"] // fails to find types
"vue/types/options.js" => ["./types/options.ts", "./types/options.d.ts", "./types/options.js"] // finds on second try
"vue/types/options.d.ts" => ["./types/options.d.ts"] // finds on first try
After this change:
"vue/types/options" => ["./types/options.d.ts", "./types/options"] // finds on first try
"vue/types/options.js" => ["./types/options.js.d.ts", "./types/options.ts", "./types/options.d.ts", "./types/options.js"] // finds on third try
"vue/types/options.d.ts" => ["./types/options.d.ts.d.ts", "./types/options.d.ts"] // finds on second try
Since the most common case is to write these imports without the extensions, I thought it would be best to write this as ["./types/*.d.ts", "./types/*"]. This makes the common case more direct to resolve, at the expense of a few more non-sensical checks (.d.ts.d.ts and .js.d.ts) in the uncommon case where we were importing with an extension.
|
@yyx990803 Hey Evan is there any chance that this could be considered for a future vue 2.7.16 release before it reaches the end of the support window? This issue has been starting to show up more and more as a blocker recently as I have been running into cases where some libraries (such as Got) require configuring "moduleResolution": "bundler", but a bunch of stuff in the vue 2.7 ecosystem stops working when you enable this option. The actual changes required to fix things are minimal and just amount to small changes to the package.json exports map, and I tried to be extra thorough in my descriptions of what these things do to provide confidence in the overall safety of making these changes. For more in depth information, please see the attached issue (which contains a reproducer), or the commit message here |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
mainbranch for v2.x (or to a previous version branch)fix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information:
Each commit contains a pretty lengthy description to try to make things as clear as possible. Rather than reproduce that here, the short version is that this PR contains two bug fixes:
"type": "module"context when using "moduleResolution": "bundler" | "node16"