-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: support ES modules without file extension within module
scope
#49531
Conversation
Review requested:
|
Hi, thanks for this @LiviaMedeiros. A few things that I think are needed:
|
doc/api/esm.md
Outdated
@@ -1008,7 +1002,7 @@ _isImports_, _conditions_) | |||
> 5. Let _packageURL_ be the result of **LOOKUP\_PACKAGE\_SCOPE**(_url_). | |||
> 6. Let _pjson_ be the result of **READ\_PACKAGE\_JSON**(_packageURL_). | |||
> 7. If _pjson?.type_ exists and is _"module"_, then | |||
> 1. If _url_ ends in _".js"_, then | |||
> 1. If _url_ ends in _".js"_ or lacks file extension, then |
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.
type module controls .js, but there’s ways to use ESM without type module - it shouldn’t be overloaded to also control extensionless, and there needs to be a way to use extensionless ESM without type module imo.
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.
Please elaborate if you mean a need to reword this or suggest changes to the implementation.
The type
controls how any JS code is interpreted. The exceptions are files with explicitly typed extensions (.cjs
, .mjs
, maybe future formats) and with unknown extensions (these should throw to ensure that it breakage is minimized if we add future formats).
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 suggesting an additional package.json key to control extensionless. “type” only controls how .js is interpreted, not “any” js code.
Changing to
semver-major
No, this must come in separate PR. I'll make a minimal follow-up implementation for extensionless |
I don’t think we can land this without some consensus that the other can land. If you want to make two PRs and they both get approvals and then a third PR combines the two and can land, or some other process, that’s fine; but unless we know that landing this won’t foreclose the possibility of extensionless Wasm in the future, or we have consensus that we’re okay with potentially not supporting extensionless Wasm if no detection algorithm can be found, then I don’t think we can land this on its own. |
doc/api/esm.md
Outdated
### Mandatory file extensions | ||
|
||
A file extension must be provided when using the `import` keyword to resolve | ||
relative or absolute specifiers. Directory indexes (e.g. `'./startup/index.js'`) | ||
must also be fully specified. | ||
|
||
This behavior matches how `import` behaves in browser environments, assuming a | ||
typically configured server. |
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 is referring to import
, not entry points, and shouldn’t be removed. I think the docs for this PR would more or less match the reverse of the changes in #31415.
@@ -74,7 +73,7 @@ function extname(url) { | |||
*/ | |||
function getFileProtocolModuleFormat(url, context, ignoreErrors) { | |||
const ext = extname(url); | |||
if (ext === '.js') { | |||
if (ext === '.js' || ext === '') { |
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 think we’re aiming to change how extensionless files are handled as entry points, not in general from any import
.
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.
How come? Both ESM with other protocols and CJS have no problems with extensionless imports.
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.
It would be very weird to have a file parsed as ESM only some of the time, I much prefer the current approach.
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.
In the original implementation before it was limited, we supported extensionless per "type"
only for entry points, not for any import
, per #31415 (comment). We’ve never supported extensionless for import
. I could search around for why there was the original limitation to make this only for entry points, but I’d bet there was a good reason. There’s also not really a use case for import
of extensionless files.
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’ve never supported extensionless for
import
In the comment you just linked, you say:
support for extensionless files in import was added only last month, in #31021.
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.
Okay so we didn’t never support it, but it wasn’t part of the original implementation. It was briefly added and then reverted. Probably because of this: #31021 (review)
One of the major features of the ES module resolver is being able to support new file extensions in future, which is reliant on the fact that we always throw for unknown file extensions.
Giving special treatment to
isMain
was how we did this while ensuring compatibility with existingbin
s.Happy to flesh this out further, but I don’t think we should lose that extension property for the ESM resolver.
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 dug through a lot of old issues and consulted @guybedford to try to rediscover why we limited extensionless support to entry points in the initial implementation, and why we removed even that in #31415. Basically, we wanted to preserve the ability for future extensionless Wasm entry points. And I think we still do; that goal hasn’t changed, and #49540 is a viable way to achieve that goal. I think years ago we weren’t aware of or really thinking about the “disambiguate based on magic bytes” solution, and so we just locked down the design space in the hope that we’d figure something out eventually. And I think the magic bytes approach is that something, and so it can open up the ability for both extensionless ESM entry points and extensionless Wasm entry points.
As for import
of extensionless files, there would be the similar question of how to disambiguate an import
of an extensionless JavaScript file from an extensionless Wasm file. The simplest solution is probably to just take same approach that we use for entry points, looking at magic bytes. Currently import
of Wasm is gated behind the separate flag --experimental-wasm-modules
, so it doesn’t matter all that much.
But I think we can support import
of extensionless JavaScript. JavaScript can be the default for an import
statement, where “extensionless = JavaScript with the format determined by type
” for import
like for entry point, with other potential formats needing a way to disambiguate. Those new types would either need explicit extensions (the most likely and obvious disambiguation) or they would need mandatory import attributes (like JSON) or they would need magic bytes; some way to disambiguate them from JavaScript. But I think we can unlock import
of extensionless JavaScript without much risk of that prohibiting future enhancements.
We should land this when we have consensus on this. d78a9da has Wasm. |
We can’t land this on its own without either consensus on an approach for Wasm, or consensus on agreeing that we don’t care if we foreclose the possibility of extensionless Wasm in the future. #31415 set the precedent that we want to preserve that possibility. We can certainly revisit that decision, but until we do I’m assuming that it still stands. A “go big” PR would avoid that problem, since it would answer the question of how to handle extensionless Wasm. It would also avoid the problem of adding several flags, since it could combine multiple behaviors behind just one. @LiviaMedeiros Do you mind attempting a single PR that includes everything? And if it can’t land for some reason then we can revisit. To copy from #49432 (comment), here’s what I think the big PR should include:
And this should all get put behind a flag. In #49541 I proposed What do you think? |
No, this is a separate feature that should be there no matter what flags are landed or enabled. This also should be separated from Wasm support, so we can easily backport it, if needed.
What is not in consensus about Wasm? |
I don’t think these statements are quite right. In my opinion:
@LiviaMedeiros do you have an objection to potentially doing everything in one PR? I was assuming not, since #49295 was a big PR with several ESM-first features grouped together. I can understand wanting to land separate PRs as a process concern, to maybe divide up the work and ease reviewing and such; we can do that and add “don’t land” labels so that none of the PRs get released until they’re all landed (and if any get blocked or abandoned, we’d have to revert the earlier ones that landed on |
Given that this is If the concern is about potential breakage, let's try CITGM run on this. It can't be combined with any flags that change |
One of the “larger” features is supporting extensionless files as ESM where there is no We don’t need to split it out; I was suggesting that to be charitable. We could just land everything together in the big flag.
This is the approach that I think is most likely to get majority support. We’ll discuss tomorrow in nodejs/loaders#160 if you’d like to attend. If individual components have merit on their own, they can be split out under separate flags; though if anything it might make more sense to further consolidate, like the suggestion to make |
This comment was marked as outdated.
This comment was marked as outdated.
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 don’t think I was clear enough in my previous objection, so I’ll spell it out more here. I’m objecting to this based on all three of the bullet points from #49531 (comment):
- I think this needs to land under an experimental flag at first.
- That flag should encompass all the “default as ESM” behaviors we’re considering, not just extensionless files in a
module
scope. - That flag must include a solution for extensionless Wasm, such as magic bytes.
Once the above lands, we could consider creating a separate flag just for extensionless files as ESM (or even extensionless files within module
scopes as ESM, though I don’t see the point of that) but the above needs to land first so that we’re sure we’ve solved the extensionless Wasm use case; and so that users have a single flag to handle extensionless files as ESM both within package scopes and without.
I agree with @GeoffreyBooth. |
I strongly disagree and I find a PR block that doesn't provide a reason why this PR can't land and instead proposes unfeasible alternative to be unreasonable. To summarize the main problem of alternative, the effect of #49432 can work only outside of There is no reason to not allow developers to use extensionless ES modules inside of Additionally, if we combine the flags, the same problem will occur to extensionless Wasm that directly depends on this feature. This PR does solve these problems, and opens path for both extensionless Wasm and extensionless ESM outside of pjson scopes, effectively achieving the same goal. Without flags with side effects, limitations, nor making CJS projects less usable. Semverity is up to TSC; it also can land without runtime flag (#49629 is unflagged version of this). |
I think we have covered this enough in #49494 not to be the case. None of the new flags will affect I prefer this change to happen alongside the change to the I don't understand why the above is unreasonable. |
That's amazing, I'll update the summary assuming this and #49432 (comment).
We usually don't group multiple different flags in one to minimize testing, do we? Otherwise we could already have some For the record, this one can be landed without any flag, unless there is a clear reason to not. So far, there was no example of potential breakage that would make it worse than |
Well specifically, redefining how The reason to group them together is that from the user perspective, extensionless files are extensionless files; it’s not a separate “feature” how extensionless files are treated:
I agree that we can land the “how extensionless files are treated within a
We haven’t traditionally tried to control the scope of flags. We had |
From a user perspective with pjsonless extensionless scripts, I absolutely can't feel very bad about extensionless files outside of scopes for a very simple reason: they already work! But as CJS, which is completely understandable. If, as user, I really want my loose extensionless scripts to work, I would be able to do it with this PR by doing But what will really make me feel very frustrating, is this: This kind of scenario I would absolutely hate as user, because I can not comprehend WHY adding pjson doesn't help, and I have no choice than to either make my extensionless scripts not so extensionless, or make my own local copy of Also, as user I would be frustrated from just reading documentation of a flag that instead of just flipping default |
<!-- YAML | ||
added: REPLACEME | ||
--> | ||
|
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.
It's missing the stability: 1.1 Active Development
As discussed in today's TSC meeting, I'm adding |
Closing now that #49974 has landed. |
This makes interpreting extensionless local files symmetrical in
modules
andcommonjs
scopes: they are treated the same as.js
.Earlier his feature was removed in favor of eventually supporting extensionless Wasm. Turns out, extensionless Wasm support can be added without making extensionless ES modules not work.
semver-minorPRs that contain new features and should be released in the next minor version.
with
--experimental-extensionless-modules
flag unless there are objections.Either semver-majorPRs that contain breaking changes and should be released in the next major version.
follow-up that unflags this or flagless alternative that can be landed as is: #49629
Potential follow-up that adds extensionless Wasm support: #49540 (d78a9da, to be precise).
Refs: #49431