| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
Sorry, something went wrong.
|
Hi, thanks for this @LiviaMedeiros. A few things that I think are needed:
|
Sorry, something went wrong.
| > 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.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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).
Sorry, something went wrong.
There was a problem hiding this comment.
I’m suggesting an additional package.json key to control extensionless. “type” only controls how .js is interpreted, not “any” js code.
Sorry, something went wrong.
Changing to semver-major PRs that contain breaking changes and should be released in the next major version. . I guess if there will be need in this feature for earlier releases, we can backport it with flag.
No, this must come in separate PR. I'll make a minimal follow-up implementation for extensionless module/Wasm. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
| ### 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.
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.
Sorry, something went wrong.
| function getFileProtocolModuleFormat(url, context, ignoreErrors) { | ||
| const ext = extname(url); | ||
| if (ext === '.js') { | ||
| if (ext === '.js' || ext === '') { |
There was a problem hiding this comment.
I think we’re aiming to change how extensionless files are handled as entry points, not in general from any import.
Sorry, something went wrong.
There was a problem hiding this comment.
How come? Both ESM with other protocols and CJS have no problems with extensionless imports.
Sorry, something went wrong.
There was a problem hiding this comment.
It would be very weird to have a file parsed as ESM only some of the time, I much prefer the current approach.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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 existing bins.
Happy to flesh this out further, but I don’t think we should lose that extension property for the ESM resolver.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
We should land this when we have consensus on this. d78a9da has Wasm. |
Sorry, something went wrong.
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 --experimental-type=module / --experimental-type=commonjs, so we can use that for now in the PR and change it later if a better alternative emerges. What do you think? |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
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 main based on an assumption that everything would land). But based on the various PRs offered so far, a PR that puts them all together seems like it wouldn’t be so large as to be unwieldy or hard to review, and I think the issues we’ve opened so far make clear that there is likely to be a path forward for it. I or someone else can take over this task if you don’t want to do it, but since you’ve been the driving force of this so far I thought you would probably want to lead it. |
Sorry, something went wrong.
|
Given that this is semver-major, I don't see any need to put it under an experimental flag. If the concern is about potential breakage, let's try CITGM run on this. It can't be combined with any flags that change type interpretation. Otherwise it would break in all CJS-first projects in ecosystem. |
Sorry, something went wrong.
One of the “larger” features is supporting extensionless files as ESM where there is no package.json file. I don’t know what the use case is for supporting extensionless files only within package scopes. This smaller feature is so small that I’m not sure if it has merit to land on its own, or that it deserves its own flag. If anything it’s a footgun because it enables something similar to what people want but isn’t what people want. 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 --input-type control not just STDIN/eval but all ESM default behaviors. |
Sorry, something went wrong.
There was a problem hiding this comment.
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):
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.
Sorry, something went wrong.
Sorry, something went wrong.
|
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 node_modules and only with addition of changing at least how default type is interpreted, what is default --input-type. Even after the proposed "flipping defaults", there would be a backward-compatibility flag like --type=commonjs that undoes it. There is no reason to not allow developers to use extensionless ES modules inside of node_modules, in CJS-first projects that do not want default type to change, and in future-legacy projects that have to preserve the current default after the flip. 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). |
Sorry, something went wrong.
I think we have covered this enough in #49494 not to be the case. None of the new flags will affect node_modules, which will be left untouched. That's the prerequisite of all this initiative: not break the ecosystem. I prefer this change to happen alongside the change to the type default inside package.json and I think those should be controlled by the same flag to minimize the actual testing maintainers should do, e.g. "please verify everything works with node --esm". I don't understand why the above is unreasonable. |
Sorry, something went wrong.
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 --experimental-everything that enables all bleeding edge features at once. 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 semver-major. This feature doesn't remove or change anything; it only makes loading a module work in condition that otherwise throws ERR_UNKNOWN_FILE_EXTENSION. I'll open unflagged version separately to make it less confusing. |
Sorry, something went wrong.
Well specifically, redefining how type behaves is something we’re excluding from packages underneath a node_modules folder. It wasn’t proposed to restrict how extensionless files under node_modules are interpreted, because as @LiviaMedeiros points out, doing so wouldn’t be a breaking change. 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 module scope” part on its own and it wouldn’t be a breaking change. But if that landed before the user had some way of affecting extensionless files outside of a module scope, it would feel very frustrating. The extensionless files that users want to influence are the loose ones. It would be better to first ship a flag that lets them change all extensionless files, wherever they are, and then spin off from that the “within module scope” part to land on its own unflagged now that there’s already a way to influence loose ones.
We haven’t traditionally tried to control the scope of flags. We had --experimental-modules for years to enable the entire ESM implementation. I view this new “ESM-first” flag as in that vein. In some ways it’s easier to have one big flag than many little ones, as there are fewer combinations of toggles to test. |
Sorry, something went wrong.
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 echo '{"type":"module"}' > ~/my_esm_utils_that_are_in_PATH/package.json and to be happy. 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 func_a.js in my own new local directory that must have {"type":"commonjs"} pjson. Also, as user I would be frustrated from just reading documentation of a flag that instead of just flipping default type has a long list of side effects, especially if I search for a particular feature that happened to be a part of it. Like, what kind of technical issue happened that Node.js couldn't have this and that effect by default? |
Sorry, something went wrong.
| <!-- YAML | ||
| added: REPLACEME | ||
| --> | ||
|
|
There was a problem hiding this comment.
It's missing the stability: 1.1 Active Development
Sorry, something went wrong.
|
As discussed in today's TSC meeting, I'm adding dont-land-on-v20 and dont-land-on-v18 labels. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This makes interpreting extensionless local files symmetrical in modules and commonjs 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-minor PRs that contain new features and should be released in the next minor version. with --experimental-extensionless-modules flag unless there are objections.
Either semver-major PRs 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