feat(resolve): support "fallback array" in package exports field#10504
feat(resolve): support "fallback array" in package exports field#10504aleclarson wants to merge 6 commits intovitejs:mainfrom
Conversation
Closes vitejs#4439 More context: vitejs#10504
7dfb08e to
165cdef
Compare
|
I just rebased this onto |
| data, | ||
| '.', | ||
| options, | ||
| getInlineConditions(options.conditions, targetWeb) |
There was a problem hiding this comment.
Conditions that can't be inferred from Vite's resolve options are generated with getInlineConditions and passed as their own argument.
| } | ||
| try { | ||
| let entryPoint: string | undefined | void | ||
| let entryPoints: string[] = [] |
There was a problem hiding this comment.
The new resolveExports returns an array of possible module paths. It prefers to return an empty array, rather than throw an error. Vite should only throw if none of the paths in the array are found.
| getInlineConditions(options.conditions, targetWeb) | ||
| ) | ||
| if (!entryPoints.length) { | ||
| packageEntryFailure(id) |
There was a problem hiding this comment.
This comes from #8484, and it makes resolution more strict when an exports field is found. If the imported module isn't specified in exports map, avoid checking other fields (e.g. main or browser).
| if ( | ||
| targetWeb && | ||
| options.browserField && | ||
| (!entryPoint || entryPoint.endsWith('.mjs')) |
There was a problem hiding this comment.
No need for .mjs check anymore, since exports resolution is now strict.
Bug introduced in vitejs#10683 Some packages use "require" instead of "default" for CJS entry
| inlineConditions.push('browser') | ||
| } | ||
| } else if (!conditions.includes('browser')) { | ||
| inlineConditions.push('node') |
There was a problem hiding this comment.
This mirrors the browser option of lukeed's resolve.exports module (see here), but we might want to replace the ['node'] expression with [] to avoid the assumption that non-browser environment is using Node.js
d496f08 to
3c492bb
Compare
| '.', | ||
| options, | ||
| getInlineConditions(options, targetWeb), | ||
| options.overrideConditions |
There was a problem hiding this comment.
When overrideConditions is defined, only conditions in that array will be allowed. To enforce that, we must pass it as the last argument of resolveExports.
| browserField: false, | ||
| conditions: [], | ||
| overrideConditions: ['node'], | ||
| overrideConditions: ['node', 'require'], |
There was a problem hiding this comment.
This fixes a bug introduced in #10683
Some packages use "require" instead of "default" for CJS entry (eg: vitest/config)
There was a problem hiding this comment.
Actually, this bug was introduced (and now fixed) by this PR :)
The overrideConditions array is respected by @alloc/resolve.exports for all conditions (including import and require) except for default of course. So we have to define import or require explicitly in the overrideConditions option.
…in the `getInlineConditions` function.
3c492bb to
4d3c6ac
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…and avoid breaking certain `tryFsResolve` calls too (like with `es5-ext`)
|
Ok I think the failing test is unrelated |
|
This is better supported in
Appreciate the fork, snide remarks, lack of PRs or comments, and the attempt-to-swap though. Disappointing |
|
Since |
Description
Closes #4439
Adds support for "fallback arrays" (see these tests for examples).
Replaces the
resolve.exportspackage with@alloc/resolve.exports(a higher quality rewrite). I'd be fine with transferring this package to the@vitejsorganization if you'd like.Why not fix this upstream? The related issue has been open without a response from the maintainer for 7 months. I'm confident we (or just me) can support the package better. In addition, the original package values "code golfing" over readability (no TypeScript, no code formatter, confusing variable names), so it's harder for people to contribute. Also, it doesn't use Vitest for its tests.
My rewrite is also tailored for Vite's needs, so any cruft is removed and there's no need to create a new object for every
resolveExportscall. My rewrite also has 100% test coverage and handles more of the "unofficial spec" (as described by Node.js docs and Webpack docs).See here for API differences between the original and the rewrite.
Additional context
I've also merged #8484 into this. @bluwy has said he'd be willing to write tests for that part of the PR.
What is the purpose of this pull request?