fix(node-resolve): Fix package exports / imports resolution algorithm #1549
Conversation
susnux
force-pushed
the
fix/node-resolve-exports
branch
from
d65e95e
to
99c413a
Compare
|
Thanks for the PR. @guybedford is our resident resolutions expert, hoping he sees this and offers thoughts. |
There was a problem hiding this comment.
This really looks massive and appears to contain many subtle improvements. It also appears you checked against the original algorithm, is that true?
Admittedly, the TypeScript conversion both is very useful and makes comparing the diff rather hard. For other reviewers, the relevant files to look at are resolvePackageImportsExports.ts and resolvePackageTarget.ts as only those contain major changes.
I do not feel confident to 100% judge the accuracy of the algorithm, but I would hope this feature can land as it seems to fix everything that might subtly differ from the Node algorithm.
| // Assert: ends with "/" or contains only a single "*". | ||
| .filter((k) => k.endsWith('/') || k.includes('*')) |
There was a problem hiding this comment.
You say you only want patterns that contain a single *, but this will also keep patterns with multiple *, is this intended?
There was a problem hiding this comment.
This is how the algorithm is written - basically multiple ** are invalid, but we don't ban them outright in case it might be supported in future - so instead they are filtered out by omission since they can never be matched.
packages/node-resolve/src/package/resolvePackageImportsExports.ts
Outdated
Show resolved
Hide resolved
packages/node-resolve/src/package/resolvePackageImportsExports.ts
Outdated
Show resolved
Hide resolved
| // Assert: ends with "/" or contains only a single "*". | ||
| .filter((k) => k.endsWith('/') || k.includes('*')) |
There was a problem hiding this comment.
This is how the algorithm is written - basically multiple ** are invalid, but we don't ban them outright in case it might be supported in future - so instead they are filtered out by omission since they can never be matched.
|
@lukastaegert I'll leave this one up to you to merge when you're happy with the convo with @guybedford |
…orithm according to Node documentation This fixes the package exports and imports resolution algorithm by strictly following the Node API documentation. For backwards compatibility a new option `allowExportsFolderMapping` is introduced which will enable deprecated folder mappings. Test case included Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
susnux
force-pushed
the
fix/node-resolve-exports
branch
from
99c413a
to
95b7671
Compare
|
@lukastaegert @guybedford Thank you very much for your review! |
Rollup Plugin Name:
node-resolveThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
Currently the implementation of package exports / imports resolution does not follow the Node documentation as the README implies.
So this fixes the package exports and imports resolution algorithm by strictly following the Node API documentation. For backwards compatibility a new option
allowExportsFolderMappingis introduced which will enable deprecated folder mappings.This is required for packages that use export patterns with the asterisk in the middle not at the end, like this:
{ "exports": { "./component/*.js": { "import": "./dist/*.mjs", "require": "./dist/*.cjs" } } }See included test file.