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

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

Closed
Boshen opened this issue Dec 22, 2023 · 10 comments · Fixed by #429
Closed

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

Boshen opened this issue Dec 22, 2023 · 10 comments · Fixed by #429

Comments

@Boshen
Copy link

Boshen commented Dec 22, 2023

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:

  • vite - conforms to spec ✔
  • esbuild - conforms to the spec ✔
  • resolve.exports - conforms to the spec ✔
  • resolve-pkg-maps - conforms to the spec ✔
  • node.js
  • typescript - available in 5.0 with moduleResolution: bundler, does not conform to the spec according to the implementation
  • enhanced-resolve - does not conform to the spec as tested by this repo
  • rspack - undecided

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

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

Boshen commented Dec 22, 2023

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

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

@andrewbranch
Copy link

andrewbranch commented Dec 22, 2023

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
Copy link

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

@hardfist
Copy link

hardfist commented Dec 23, 2023

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
Copy link

hardfist commented Dec 23, 2023

@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

@alexander-akait alexander-akait moved this to Priority - High in webpack 5/6 Dec 28, 2023
@Boshen
Copy link
Author

Boshen commented Jan 2, 2024

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

/**
* @param {string} exp export target
* @param {boolean} expectFolder is folder expected
*/
function assertExportTarget(exp, expectFolder) {
if (
exp.charCodeAt(0) === slashCode ||
(exp.charCodeAt(0) === dotCode && exp.charCodeAt(1) !== slashCode)
) {
throw new Error(
`Export should be relative path and start with "./", got ${JSON.stringify(
exp
)}.`
);
}
const isFolder = exp.charCodeAt(exp.length - 1) === slashCode;
if (isFolder !== expectFolder) {
throw new Error(
expectFolder
? `Expecting folder to folder mapping. ${JSON.stringify(
exp
)} should end with "/"`
: `Expecting file to file mapping. ${JSON.stringify(
exp
)} should not end with "/"`
);
}
}

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

alexander-akait commented Jan 24, 2024

@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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Shipped
4 participants