webpack / enhanced-resolve

Offers an async require.resolve function. It's highly configurable.
MIT License
916 stars 186 forks source link

ESM: package.exports array target not conforming to the ESM specification #400

Open Boshen opened 6 months ago

Boshen commented 6 months ago

Repro and research: https://github.com/Boshen/test-esm-exports-array

Pasting the content here:

Resolving package.exports array target

Background

ESM can define an array as the exports target:

{
  "exports": {
    ".": [
      "-bad-specifier-",
      "./non-existent.js",
      "./existent.js"
    ]
  }
}

Given only ./existent.js is on disk, enhanced-resolved will resolve to ./existent.js while Node.js will throw a "./non-existent.js not found" error.

Explanation

The ESM specification states the following:

PACKAGE_TARGET_RESOLVE(packageURL, target, patternMatch, isImports, conditions)
...
3. Otherwise, if target is an Array, then
...
  2. For each item targetValue in target, do
    1. Let resolved be the result of PACKAGE_TARGET_RESOLVE( packageURL, targetValue, patternMatch, isImports, conditions), continuing the loop on any Invalid Package Target error.

Notice the last line continuing the loop on any Invalid Package Target error..

"Invalid Package Target Error" does not mean "File not Found".

This means the above package.json should yield ./non-existent.js instead of ./existent.js.

The reason for this is that exports is designed to resolve unambiguously without hitting the disk.

Also documented by here:

Node.js's implementation picks the first valid path (without attempting to resolve it) and throws an error if it can't be resolved. Node.js's fallback array is designed for forward compatibility with features (e.g. protocols) that can be syntactically validated:

enhanced-resolved

As documented on the Webpack website:

Instead of providing a single result, the package author may provide a list of results. In such a scenario this list is tried in order and the first valid result will be used.


At the moment of writing, I have yet to find a legitimate use case for this feature, but the behavior from Webpack will lead to people setting up their exports array for some use cases that can break future compatibility.

We have already seen this with the browser field.


All build tools has been reported with the same problem with the same discussions over and over again, linking to the issues:


Reproduce

pnpm install
bash test.sh
From Node.js:

node:internal/modules/cjs/loader:528
    throw e;
    ^

Error: Cannot find module '/test-esm-exports-array/non-existent.js'
    at createEsmNotFoundErr (node:internal/modules/cjs/loader:1070:15)
    at finalizeEsmResolution (node:internal/modules/cjs/loader:1063:15)
    at trySelf (node:internal/modules/cjs/loader:522:12)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1025:24)
    at Module._load (node:internal/modules/cjs/loader:901:27)
    at Module.require (node:internal/modules/cjs/loader:1115:19)
    at require (node:internal/modules/helpers:130:18)
    at [eval]:1:1
    at Script.runInThisContext (node:vm:122:12)
    at Object.runInThisContext (node:vm:298:38) {
  code: 'MODULE_NOT_FOUND',
  path: '/test-esm-exports-array/package.json'
}

Node.js v20.5.1

----------------------------------------
From esbuild:

✘ [ERROR] Could not resolve "test-esm-exports-array"

    <stdin>:1:7:
      1 │ import('test-esm-exports-array')
        ╵        ~~~~~~~~~~~~~~~~~~~~~~~~

  The module "./non-existent.js" was not found on the file system:

    package.json:6:6:
      6 │       "./non-existent.js",
        ╵       ~~~~~~~~~~~~~~~~~~~

  You can mark the path "test-esm-exports-array" as external to exclude it from the bundle, which
  will remove this error and leave the unresolved path in the bundle. You can also add ".catch()"
  here to handle this failure at run-time instead of bundle-time.

1 error

----------------------------------------
From enhanced-resolve:

dir: /test-esm-exports-array
specifier: test-esm-exports-array
resolved:  /test-esm-exports-array/existent.js

Notice enhanced-resolve resolved to ./existent.js, spec compliant implementations reports ./non-existent.js not found.

alexander-akait commented 6 months ago

Yeah... I think we need to do the same because other bundlers do it and we should not allow people make such mistakes, I am afraid it can be a breaking changes in some way, but in this case I want to say it is a bug fix.

Do you want to send a PR?

Boshen commented 6 months ago

Do you want to send a PR?

I can give it a try next week.

Let's also wait for a response from the TypeScript team.

alexander-akait commented 6 months ago

@Boshen Yeah, let's wait, but I am still thinking it is a strange behaviour and should work as a fallback :smile:

andrewbranch commented 6 months ago

I’m very supportive of trying to make exports handling consistent between all tools, and I think the principle Node.js is following of trying to reduce the number of FS hits required to return a resolution result is a good one. I think it would be great if bundlers matched Node.js in this case.

Has anyone checked Bun’s runtime behavior?

hardfist commented 6 months ago

@andrewbranch will Typescript align this behavior with Node.js (when moduleResolution se to bundler)?

hardfist commented 6 months ago

Has anyone checked Bun’s runtime behavior?

@andrewbranch I check bun's behavior, it seems Bun even doesn't do fallback for -bad-specifier- which is not compatible with Node.js, friendly ping @Jarred-Sumner

hardfist commented 6 months ago

@Boshen Yeah, let's wait, but I am still thinking it is a strange behaviour and should work as a fallback 😄

@alexander-akait I have a same feeling, fallback for bad-specifier but not for non-exist file is really strange and the performance reason is only theoretical, fallback for non-exist file do help simplify monorepo configuration and it seems fallback for bad-specifier even less useful than fallback for non-exist

Boshen commented 6 months ago

I don't think I can patch these two ESM problems (this issue and #399).

The current implementation is not following the ESM specification, resulting conflicting behaviors or major breaking changes that I don't know how to make decisions nor how to make the correct changes.


For example, the code for checking export target

https://github.com/webpack/enhanced-resolve/blob/58464fc7cb56673c9aa849e68e6300239601e615/lib/util/entrypoints.js#L198-L227

is different from Invalid Package Target specified in the spec.

ExportsFieldPlugin.js has be changed somehow to accept Invalid Package Target and handle it

resolver.doResolve(
    target,
    obj,
    "using exports field: " + p,
    resolveContext,
-       callback,
+   (err, result) => {
+       if (result === undefined && /* target is not "Invalid Package Target" */) {
+           callback(new Error(`Cannot find module ${obj.path}`));
+       } else {
+           callback(err, result);
+       }
+   }
);

I think the best way forward is to update the documentation around https://webpack.js.org/guides/package-exports/#alternatives, either remove it, or change it to the "fallback to typescript" use case

  "exports": {
    ".": {
      "development": "./src/index.ts",
      "default": "./dist/index.js"
    },
    "./types": {
      "development": "./src/types.ts",
      "default": "./dist/types.js"
    }
}
alexander-akait commented 5 months ago

@Boshen Sorry for delay, I think we should be align to the spec, but, yes, some developers can rely on our logic, that is why I think we can intoduce the new options to return the old behaviour and will write this in changelog, so it will allow to migrate smoothly.

Yes, such changes are sometimes painful, but non-standard opportunities are even worse.

alexander-akait commented 5 months ago

If we encounter this very often, then we may revert to the old behavior and use futureDefaults to the enable this logic for future webpack@6. I really don't know how many developers use it, but I don't think a lot...