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.

Sign up for GitHub

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

Jump to bottom

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

Closed

aleclarson wants to merge 6 commits into vitejs:main from aleclarson:feat/array-fallback-exports

Member

aleclarson commented Oct 17, 2022 •

edited

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

This was referenced Oct 17, 2022

package.json "exports" field fallback array support #4439

Closed

feat(resolve): only use exports field when defined in package.json #8484

Closed


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

165cdef

Closes vitejs#4439

More context:
  vitejs#10504

aleclarson force-pushed the feat/array-fallback-exports branch from 7dfb08e to 165cdef Compare

November 14, 2022 17:33

Member Author

aleclarson commented Nov 14, 2022

I just rebased this onto main

aleclarson commented

View reviewed changes

packages/vite/src/node/plugins/resolve.ts Outdated

+                      data,
+                      '.',
+                      options,
+                      getInlineConditions(options.conditions, targetWeb)

Member Author

aleclarson Nov 14, 2022

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

aleclarson commented

View reviewed changes

packages/vite/src/node/plugins/resolve.ts

    
            @@ -923,29 +923,28 @@ export function resolvePackageEntry(
          
                  return cached

                }

                try {

                  let entryPoint: string | undefined | void

                  let entryPoints: string[] = []

Member Author

aleclarson Nov 14, 2022 •

edited

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.

aleclarson commented

View reviewed changes

packages/vite/src/node/plugins/resolve.ts

+                      getInlineConditions(options.conditions, targetWeb)
+                    )
+                    if (!entryPoints.length) {
+                      packageEntryFailure(id)

Member Author

aleclarson Nov 14, 2022 •

edited

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).

aleclarson commented

View reviewed changes

packages/vite/src/node/plugins/resolve.ts

-                  if (
-                    targetWeb &&
-                    options.browserField &&
-                    (!entryPoint || entryPoint.endsWith('.mjs'))

Member Author

aleclarson Nov 14, 2022

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

aleclarson mentioned this pull request

refactor: resolveExports #10917

Merged

9 tasks

aleclarson added 3 commits

November 14, 2022 13:12


          refactor: overrideConditions

606f126


          fix: add module condition by default

a94200f


          fix: config bundling regression

296fe46

Bug introduced in vitejs#10683

Some packages use "require" instead of "default" for CJS entry

aleclarson commented

View reviewed changes

packages/vite/src/node/plugins/resolve.ts

+                    inlineConditions.push('browser')
+                  }
+                } else if (!conditions.includes('browser')) {
+                  inlineConditions.push('node')

Member Author

aleclarson Nov 14, 2022

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

aleclarson force-pushed the feat/array-fallback-exports branch from d496f08 to 3c492bb Compare

November 14, 2022 18:36

aleclarson commented

View reviewed changes

packages/vite/src/node/plugins/resolve.ts

+                      '.',
+                      options,
+                      getInlineConditions(options, targetWeb),
+                      options.overrideConditions

Member Author

aleclarson Nov 14, 2022

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.

aleclarson commented

View reviewed changes

packages/vite/src/node/config.ts

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

Member Author

aleclarson Nov 14, 2022 •

edited

This fixes a bug introduced in #10683

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

Member Author

aleclarson Nov 14, 2022 •

edited

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.


          fix: respect overrideConditions and isRequire

4d3c6ac

…in the `getInlineConditions` function.

aleclarson force-pushed the feat/array-fallback-exports branch from 3c492bb to 4d3c6ac Compare

November 14, 2022 18:47

This comment was marked as outdated.

Sign in to view

aleclarson added the p3-significant label


          fix: try resolveExports call with postfix included

bb4fbd0

…and avoid breaking certain `tryFsResolve` calls too (like with `es5-ext`)

aleclarson requested review from sapphi-red and bluwy

November 14, 2022 19:21

Member Author

aleclarson commented Nov 14, 2022

Ok I think the failing test is unrelated

aleclarson mentioned this pull request

fix(resolve): make tryNodeResolve more robust #9170

Closed

4 tasks

aleclarson mentioned this pull request

feat: support ESM subpath imports #7770

Merged

9 tasks

mrm007 mentioned this pull request

Curious behaviour when loading CJS/ESM with vite.ssrLoadModule #11385

Closed

7 tasks

vexkiddy mentioned this pull request

Vite4 regression - TypeError: Cannot read properties of undefined #11299

Closed

7 tasks

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

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 closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment