Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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" field #34117
module: package "imports" field #34117
Changes from 6 commits
d9f7c01
5875d53
105b7f2
8fe68f7
1dc83f5
ceffe4c
2570191
ec4b7a4
eb34788
6a69697
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
we might want to add that fragments and queries are not preserved for non-directory paths.
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 assume they are neither preserved nor removed..? It may make sense to call out that for bare specifiers
?
/#
/etc. are treated like any other character if we don't do that already.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.
looking at the markdown algorithm they would be removed it seems.
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 tried to find the part where it removes anything from the specifier and I can't find a reference to "?"/"#" or "query"/"fragment". According to my reading of the markdown algorithm (and memory of the spec), the following should work:
The import in
entry.mjs
should load./node_modules/dep/package.json/foo.mjs?some=query
. It does honor the entire specifier, including the bits that happen to follow a question mark. So "not preserved" could be misunderstood I think. Similar behavior can also be observed in directory "exports", so this isn't specific to exact match: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.
the point is that
would not carry
?
or#
parts of the URL from input to right hand side. It also needs those to match the left hand side if they are present. I am not willing to state that these left hand side strings are not URL based/like as that goes back to my problem with putting#
in the exports field of making the switch of being an opaque value/string quite confusing and not matching any other specifier. I do think this is a difference that was fine in "exports" since it is a single location/workflow for the exception but as we grow the usage of these non relative non propagating specifiers we need to document the actual workflow. Even reading your comment above I am unclear on your meaning which is concerning on its own.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.
@bmeck like import maps, the left hand side is always a direct string replacement, so the only difference remains the standard CJS v ESM resolvers handling resolution differently for those cases, but this applies equally to any absolute or relative paths - imports and exports targets are no different.
@jkrems note that I have disallowed
#/
as a mapping prefix explicitly so your example wouldn't work and instead give an invalid specifier error. I thought it seemed sensible to lock down these base cases, but happy to reconsider.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.
OT: This algorithm is pretty much un-reviewable at this point (at least to me). I don't think it's in scope for this PR but maybe the answer is to move it to its own page and rethink how it's organized?
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'm totally open to suggestions. My main concerns are keeping the API nature of the docs straightforward and having a nice link to this spec as it should be a reference. I do generally not like having it in a details element because it makes it difficult to link this spec without it being overlooked so I'd be all for a dedicated page actually.