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
module: package imports targets outside package #52641
base: main
Are you sure you want to change the base?
Conversation
this allows the imports field of package.json to target a location outside of the package boundary. doing so allows for imports to work in directories just altering things like the type field of package.json and to enable monorepo workspace workflows.
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, thanks for contributing this!
@@ -485,7 +485,7 @@ where `import '#dep'` does not get the resolution of the external package | |||
file `./dep-polyfill.js` relative to the package in other environments. | |||
|
|||
Unlike the `"exports"` field, the `"imports"` field permits mapping to external | |||
packages. | |||
packages and locations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does https://nodejs.org/api/esm.html#resolution-algorithm-specification need to be updated as well?
@@ -414,7 +416,8 @@ function resolvePackageTargetString( | |||
const resolvedPath = resolved.pathname; | |||
const packagePath = new URL('.', packageJSONUrl).pathname; | |||
|
|||
if (!StringPrototypeStartsWith(resolvedPath, packagePath)) { | |||
// if (mustBeInternalTarget && !StringPrototypeStartsWith(resolvedPath, packagePath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure why this is here.
// if (mustBeInternalTarget && !StringPrototypeStartsWith(resolvedPath, packagePath)) { |
@@ -465,14 +468,15 @@ function isArrayIndex(key) { | |||
* @param {boolean} internal - Whether the package is internal. | |||
* @param {boolean} isPathMap - Whether the package is a path map. | |||
* @param {Set<string>} conditions - The conditions to match. | |||
* @param {boolean} mustBeInternalTarget - If the target must be in the package boundary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bit much for a JSDoc comment, but is it possible to briefly explain when we would want this restriction? Is it like If the target must be in the package scope: yes for "exports", no for "imports"
?
@@ -26,6 +26,10 @@ const { requireImport, importImport } = importer; | |||
['#subpath//asdf.asdf', { default: 'test' }], | |||
// Double slash | |||
['#subpath/as//df.asdf', { default: 'test' }], | |||
// Target steps below the package base | |||
['#belowbase', { default: 'belowbase' }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is what it was called previously, but why is this “below” the base and not above the base?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RafaelGSS does this needs to do anything specific for permissions?
@@ -587,6 +591,7 @@ function packageExportsResolve( | |||
const resolveResult = resolvePackageTarget( | |||
packageJSONUrl, target, '', packageSubpath, base, false, false, false, | |||
conditions, | |||
true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's not new in this PR, but I'm not a big fan of bare boolean arguments like this where someone can't tell at a glance what the true
or false
means when looking at the callsite. For this, adding comments like... the following example would be helpful:
const resolveResult = resolvePackageTarget(packageJSONUrl, target, '', packageSubpath, base,
false /* pattern */,
false /* internal */,
false /* isPathMap */,
conditions,
true /* mustBeInternalTarget */);
But not blocking for this PR.
this allows the imports field of package.json to target a location outside of the package boundary. doing so allows for imports to work in directories just altering things like the type field of package.json and to enable monorepo workspace workflows.
Node already allows resolving to external packages for
"imports"
; this fixes some usability problems of mixing monorepos and directories containing a{"type":"module"}
(or vice versa)package.json
preventing the ability to properly use"imports"
to resolve to reasonable locations. It does not enable..
in a specifiers to escape still per the tests it must be in the target of thepackage.json
to allow escaping.An example of a case where this feature is high friction is:
Note: this still requires duplication of imports for this use case with this PR.
Another example is in a monorepo situation: