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

Exports field trees with top-level conditions result in a misleading error message #325

Open
Sidnioulz opened this issue Jan 12, 2022 · 10 comments

Comments

@Sidnioulz
Copy link

Sidnioulz commented Jan 12, 2022

When the exports field of a package has top-level conditions leading to subpath conditions, enhanced-resolve does not parse the subpath conditions. Following a discussion with the maintainer, this is expected behaviour as that syntax is not supported by Node.js either.

However, enhanced-resolve parses the exports field without failing, and later on in a Webpack compilation, Webpack fails to resolve subpath imports. Instead, enhanced-resolve should detect this syntax and throw an error message somewhere around buildExportsFieldPathTree. The reasoning is that it's very hard to understand why subpath imports fail otherwise, because the Webpack and Node.js documentations don't explicitly state that this use of exports is unsupported.

Original ticket

(original title: Exports field trees with top-level conditions are not supported)

From my understanding, enhanced-resolve is supposed to know how to handle the exports field of package.json, including exports with conditional names, as there are checks for conditional mappings in the code.

Everything in this ticket relates to the lib/util/entrypoints.js file.

In the buildExportsFieldPathTree, we can see that exports that do not start with a dot are rejected. Conditional mappings are checked after the building of the treeRoot, so this means that exports with top-level conditions are wrongly assumed to be incorrect syntax. This results in treeRoot containing a single file entry that matches only . and has the entire exports field as its content.

package.json:

	"exports": {
		"import": {
			".": "./esm/index.js",
			"./*": "./esm/*.js"
		},
		"require": "./build/bundle.js"
	},

Webpack.config.js:

  config.resolve.conditionNames = ['import', 'node', 'default']

Obtained treeRoot:

{
  children: null,
  folder: null,
  wildcards: null,
  files: Map(1) { '' => { import: { ".": "./esm/index.js", "./*": "./esm/*.js" }, require: './build/bundle.js' }
}

As a consequence, imports of the form @org/mylib work, but @org/mylib/foo fail. Should you need a reproduction example, the one written in the Webpack 5 documentation does not work: https://webpack.js.org/guides/package-exports/#providing-commonjs-and-esm-version-stateful.

I am not sure what the appropriate approach is, hence the lack of patch.

One could perform a conditional mapping match before building treeRoot, and building the first matching sub-tree, resulting in a treeRoot like so:

{
  children: null,
  folder: null,
  wildcards: Map(1) { '' => './esm/*.js' },
  files: Map(1) { '' => './esm/index.js' }
}

Or one could map conditional trees into trees with a top-level relative path and conditional leaves, resulting in something like:

{
  children: null,
  folder: null,
  wildcards: Map(1) { '' => './esm/*.js' },
  files: Map(1) { '' => { import: "./esm/index.js", "require": "./build/bundle.js" } }
}

Or the treeRoot structure itself could be recursive, though I've no idea what impact this would have on the rest of the code.

@Sidnioulz
Copy link
Author

Workaround

If you are affected by this bug, you can instruct Webpack to force enhanced-resolve to use a specific condition, by duplicating the information in package.json under a new name. This only works if you own the library whose exports are not properly parsed.

In my case, to force enhanced-resolve to use the import subtree, I added this to my imported library's package.json:

	"exports": {
		"import": {
			".": "./esm/index.js",
			"./*": "./esm/*.js"
		},
		"require": "./build/bundle.js"
	},
	"basicExports": {
		".": "./esm/index.js",
		"./*": "./esm/*.js"
	},

Then, in my webpack.config.js:

	config.resolve.exportsFields = ['basicExports', 'exports']

Which results in basicExports being used and parsed successfully. It allows me to use the same dual CJS/ESM libraries with Rollup (no setup required), CJS Node code (no setup required), and Webpack 5 (with the above workaround).

@alexander-akait
Copy link
Member

Can you provide small reproducible example to investigate?

@Sidnioulz
Copy link
Author

@alexander-akait thanks for your quick reply!

You'll find a MWE here: https://github.com/Sidnioulz/BUGREPORT-enhanced-resolve-exports.

You'll need to run yarn on both folders. The webpack config file contains the commented out workaround and my trimmed down utils package contains the workaround basicExports field too.

@alexander-akait
Copy link
Member

alexander-akait commented Jan 12, 2022

That is interesting, because if you run code without webpack, i.e. just node src/index.mjs (rename js to mjs and use Node.js v16) you will get the same problem

@alexander-akait
Copy link
Member

alexander-akait commented Jan 12, 2022

But if you change it to:

".": "./esm/index.js",
"./*": "./esm/*"

You will get:

export default Math.random
^^^^^^

SyntaxError: Unexpected token 'export'

Expected, because we don't have type: "module", but as you can see Node.js loads file

@alexander-akait
Copy link
Member

alexander-akait commented Jan 12, 2022

Please use:

"exports": {
  ".": {
    "import": "./esm/index.js",
    "require": "./build/bundle.js"
  },
  "./*": {
    "import": "./esm/*.js",
    "require":  "./esm/*.js"
  }
},

If you use "import isFunction from '@ljn/utils/type/isFunction'; (i.e. without js at the end)

Or avoid .js in /* if you use import isFunction from '@ljn/utils/type/isFunction.js' (i.e. with js at the end).

Note - ./* with require should provide commonjs, not esm, looks like it was designed that way, i.e. Conditional exports should be always string, not objects with sub paths, so you can't forget to add other conditionals for other files, like I pointer above for ./*, I don't found any docs about it in https://nodejs.org/api/packages.html#package-entry-points and was a bit surprised too, but after rewriting it on example above I see why it was not allowed, because in your example you lack commonjs version of sub paths, that can be a problem...

@Sidnioulz
Copy link
Author

Just some additional context: we do manage extension matching in our Webpack and Rollup configs. Our node callees always use the CJS code for now which allows us to not go full module, and to import CJS code in our ESM builds (also covered by our Rollup / Webpack configs). Also, all our JSX, TS and TSX code is transpiled to JS before shipping. So, hopefully the adding or removing of file extensions should be a separate issue (though it does pollute my MWE a little bit, sorry for that).

(Also, we don't support CJS subpaths as we export our code in ESM format, so it wouldn't work without transpiling, and only our backend projects use require() so they don't want to add babel for that; the reason we're doing dual builds is for better code splitting in our frontend than the terser can achieve on CJS code, so we don't care about this limitation. This is why my MWE did not account for this scenario, though you are right that it will not work).

So, the Webpack doc link I provided in the report (https://webpack.js.org/guides/package-exports/#providing-commonjs-and-esm-version-stateful) nests conditional exports, and the Node.js doc (https://nodejs.org/api/packages.html#nested-conditions) also has nested exports, but in both cases, there is no subpath handling at the end.

What you are saying is that, if I want to use both conditional exports and subpath exports, the subpaths have to be top-level. Did I understand properly? I can't find this info on the Node.js doc but, if that is how it is supposed to work, should I open reports against Webpack and Node.js documentation to clarify this? Or is it still worth doing the mapping from my MWE code to yours in enhanced-resolve (I might not be the only one who interprets the doc that way)?

@Sidnioulz
Copy link
Author

At the very least, if you decide that the bug report should be closed, would it be possible to detect this edge case and provide a more precise error message? It took me a whole day of debugging and reading source code to find why this was failing, so earlier detection would be very useful.

@alexander-akait
Copy link
Member

What you are saying is that, if I want to use both conditional exports and subpath exports, the subpaths have to be top-level. Did I understand properly?

As you can see it is impossible due Node.js logic, we don't add something new here, just re-implement logic

I can't find this info on the Node.js doc but, if that is how it is supposed to work, should I open reports against Webpack and Node.js documentation to clarify this?
Or is it still worth doing the mapping from my MWE code to yours in enhanced-resolve (I might not be the only one who interprets the doc that way)?

I think will be great to improve it on both sides - Node.js and webpack. In fact, you can always write your own plugin and make it work, but as you can imagine, this is not a good idea, since other bundlers most likely will not support it.

At the very least, if you decide that the bug report should be closed, would it be possible to detect this edge case and provide a more precise error message?

It is good idea. Node.js outputs misleading error message too. I usually start debugging with Node.js logic and when compare it with webpack logic, this allows you to quickly understand where the error is.

Also if you don't need provide ESM support for some sub paths you can use:

"exports": {
  ".": {
    "import": "./esm/index.js",
    "require": "./build/bundle.js"
  },
  "./*": {
    "import": "./esm/*.js",
    "require":  null
  }
},

@Sidnioulz
Copy link
Author

Thank you!

I'll rework the title and body of the issue to make it about improving the error message.

@Sidnioulz Sidnioulz changed the title Exports field trees with top-level conditions are not supported Exports field trees with top-level conditions result in a misleading error message Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants