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

feat(resolve): support "fallback array" in package exports field #10504

Closed

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Oct 17, 2022

Description

Closes #4439

Adds support for "fallback arrays" (see these tests for examples).

Replaces the resolve.exports package with @alloc/resolve.exports (a higher quality rewrite). I'd be fine with transferring this package to the @vitejs organization 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 resolveExports call. 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@aleclarson
Copy link
Member Author

I just rebased this onto main

data,
'.',
options,
getInlineConditions(options.conditions, targetWeb)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditions that can't be inferred from Vite's resolve options are generated with getInlineConditions and passed as their own argument.

@@ -923,29 +923,28 @@ export function resolvePackageEntry(
return cached
}
try {
let entryPoint: string | undefined | void
let entryPoints: string[] = []
Copy link
Member Author

@aleclarson aleclarson Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member Author

@aleclarson aleclarson Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for .mjs check anymore, since exports resolution is now strict.

@aleclarson aleclarson mentioned this pull request Nov 14, 2022
9 tasks
inlineConditions.push('browser')
}
} else if (!conditions.includes('browser')) {
inlineConditions.push('node')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'.',
options,
getInlineConditions(options, targetWeb),
options.overrideConditions
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -968,7 +968,7 @@ async function bundleConfigFile(
mainFields: [],
browserField: false,
conditions: [],
overrideConditions: ['node'],
overrideConditions: ['node', 'require'],
Copy link
Member Author

@aleclarson aleclarson Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a bug introduced in #10683

Some packages use "require" instead of "default" for CJS entry (eg: vitest/config)

Copy link
Member Author

@aleclarson aleclarson Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@aleclarson

This comment was marked as outdated.

@aleclarson aleclarson added the p3-significant High priority enhancement (priority) label Nov 14, 2022
…and avoid breaking certain `tryFsResolve` calls too (like with `es5-ext`)
@aleclarson
Copy link
Member Author

Ok I think the failing test is unrelated

@lukeed
Copy link

lukeed commented Jan 15, 2023

This is better supported in resolve.exports@next which will be released as 2.0 stable in the next few days. Errors are still thrown (required by spec) and a string or string[] is returned, depending on whether or not the exports/imports matched entry was defined as an array. Resolved arrays are recursively looped & filtered for matched, truthy values.

Replaces the resolve.exports package with @alloc/resolve.exports (a higher quality rewrite). I'd be fine with transferring this package to the @vitejs organization if you'd like.

Why not fix this upstream? The lukeed/resolve.exports#17 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.

Appreciate the fork, snide remarks, lack of PRs or comments, and the attempt-to-swap though. Disappointing

@bluwy
Copy link
Member

bluwy commented Feb 14, 2023

Since resolve.exports already supports this by default, I think we can close this for now. We might need a separate discussion if we want to check the fs if an array is returned. Since node.js doesn't do that by default, I'm leaning towards Vite not doing that too. Of course this could be further discussed later.

@bluwy bluwy closed this Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

package.json "exports" field fallback array support
3 participants