-
Notifications
You must be signed in to change notification settings - Fork 43
File extension/directory index resolution in ESM #268
Comments
Thank you for opening this issue and listing everything out 😄 I'd like to pick the option that obstructs the fewest use cases. Additionally, I don't think that node should push things like browser conformance on users, given that it isn't a browser and that a huge section of the ecosystem feels this is a useful and valid feature, that that it's been part of the ecosystem for ten years. It is absolutely valid to want to continue to use resolution, and if resolution exists by default it doesn't stop those who don't want to use it from not using it. If you put full filenames in, you can be browser compatible and avoid what you judge to be performance issues, all without needlessly blocking other people's preferences. My point in all the above is that I don't think either side of this disagreement is hurt by leaving this in. I'd be very open to adding options to disable the feature, but I really think everyone gets what exactly what they want by leaving it on by default. I'm hopeful we can compromise :) |
I think the primary argument in favour of extension-probing is continuity with the old module system. It preserves those conventions. The main consequence of this decision will be to influence the conventions used by the modules written and published to npm after we release/unflag. Defaults are very powerful, so we need to ask ourselves not "what did we do before?" but "what do we want to encourage in the future?". On this topic, my opinion is that we ought to promote convergence with Browsers (and other emergent JS platforms). I accept nothing compels us to strive for this, and this is a subjective viewpoint, and that others are happy for Node and Browser to diverge. We've covered this extension-probing a few times over the years without killer-arguments and without concensus. Maybe we're due a binding vote on this? I think a decision (any decision) will be healthy for the working group and the ecosystem. |
Shipping ESM without automatic resolution isn't necessarily making a decision to not support automatic resolution in ESM ever; support can always be added at any time via PR. Indeed that might be preferable, as then a much broader constituency can debate the merits of encouraging browser compatibility versus providing the convenience of automatic resolution. In contrast, shipping with automatic resolution would be making a decision, as later removing it would mean a breaking change that would affect a lot of users. ESM would be stuck with it forever like CommonJS is. |
Yes, it is - shipping it later is a breaking change. For defaults, at least, we largely have only one shot at this - everything we want needs to be included at the outset, or it likely won’t ever be able to be added. I’ll also point out; without extension resolution, coffeescript could never have taken off, as migrating a file to or from coffeescript would have been a breaking change. CJS isn’t “stuck” with it, it has massively benefited from it, and continues to do so. |
Thanks @GeoffreyBooth for this review. The only thing I would add is that I found the two questions presented here to be highly coupled: if we are going to allow directory imports, then we will probably want to do so using some kind of index plus file extension search, as with |
@ljharb can you please expand on why adding it later is a breaking change?
…On Tue, Feb 19, 2019, 12:30 PM Kevin Smith ***@***.***> wrote:
Thanks @GeoffreyBooth <https://github.com/GeoffreyBooth> for this review.
The only thing I would add is that I found the two questions presented here
to be highly coupled: if we are going to allow directory imports, then we
will probably want to do so using some kind of index plus file extension
search, as with require. (The other option would be to use/abuse
package.json for directory imports.) If we decided to support file
extension lookup for directory imports, then it we might as well support it
for non-directory lookups as well.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#268 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAecV-xmD6_r6dy51qmDiogJHdc5ZYAaks5vPDSbgaJpZM4bCUDW>
.
|
With extension lookup matching cjs, an extensionless file next to an extensioned file of the same name matches the extensioned version first; without it, it matches only the extensionless file. Adding extension lookup breaks this case, unless I’m misunderstanding something. We could of course deviate from how cjs does it and be nonbreaking, but that wipes out many of the reasons to do it (altho not all). |
@ljharb You mean
|
As far as I’m aware, and someone please correct me if I’m wrong: In ESM, This was done to avoid breaking changes as new extensions are added in the future. For example in CommonJS I think So Edit: I’m referring to ESM mode in our current new implementation, not the ESM spec itself. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I'll also highlight that this is one of the reasons why I'd love to see the extension-magic go away. Not even the members of this WG can say with certainty how it works (at least not without looking things up). |
Just to build off of what @MylesBorins wrote, in our current ✦ ls -1
extensionless
test1.mjs
test2.mjs
unknown.ext
✦ cat test1.mjs
import './extensionless'
✦ cat test2.mjs
import './unknown.ext'
✦ ~/Sites/ecmascript-modules/node --experimental-modules ./test1.mjs
(node:79626) ExperimentalWarning: The ESM module loader is experimental.
internal/assert.js:5
require('assert')(value, message);
^
AssertionError [ERR_ASSERTION]: Code: ERR_UNKNOWN_FILE_EXTENSION; The provided arguments length (2) does not match the required ones (1).
at assert (internal/assert.js:5:22)
at getMessage (internal/errors.js:223:3)
at new NodeError (internal/errors.js:153:13)
at Loader.resolve [as _resolve] (internal/modules/esm/default_resolve.js:70:13)
at Loader.resolve (internal/modules/esm/loader.js:65:33)
at Loader.getModuleJob (internal/modules/esm/loader.js:136:40)
at ModuleWrap.promises.module.link (internal/modules/esm/module_job.js:38:40)
at link (internal/modules/esm/module_job.js:37:36)
✦ ~/Sites/ecmascript-modules/node --experimental-modules ./test2.mjs
(node:79639) ExperimentalWarning: The ESM module loader is experimental.
internal/assert.js:5
require('assert')(value, message);
^
AssertionError [ERR_ASSERTION]: Code: ERR_UNKNOWN_FILE_EXTENSION; The provided arguments length (2) does not match the required ones (1).
at assert (internal/assert.js:5:22)
at getMessage (internal/errors.js:223:3)
at new NodeError (internal/errors.js:153:13)
at Loader.resolve [as _resolve] (internal/modules/esm/default_resolve.js:70:13)
at Loader.resolve (internal/modules/esm/loader.js:65:33)
at Loader.getModuleJob (internal/modules/esm/loader.js:136:40)
at ModuleWrap.promises.module.link (internal/modules/esm/module_job.js:38:40)
at link (internal/modules/esm/module_job.js:37:36) This could probably use a better error message, but it seems to me that Myles is right, that adding extension searching later would not be a breaking change. |
@GeoffreyBooth we should potentially move forward in our implementation of support files without file extensions at all (longer term)... but at least in the short term making extensions required is a good enough first step to leaving the design space open. This could potentially break some workflows that currently exist for scripts that rely on a shebang... specifically that many of these files often do not have an extension as they want to look like a binary |
Also the language in the Node docs on
“Since the module system is locked, this feature will probably never go away” sounds to me like this author, at least, wishes it could go away; presumably because of the performance cost that it puts on Node. I know that performance is a very high priority of the Node project overall, so it wouldn’t surprise me if the Node core team would choose better performance over developer convenience, irregardless of any browser equivalence goals. If given a chance to weigh in, they might welcome the chance to make extension searching “go away” as part of the transition to ESM. And if not, it can be added to ESM later. But I think we should punt this decision to the larger group. There’s nothing overly technical about this that makes modules specialists better equipped to make this decision than core Node contributors. |
Per the proposal, at least, extensionless files would still be supported as initial entry points, just not as the specifier of an |
@GeoffreyBooth note that those docs aren't suggesting extension lookup should go away, they're suggesting allowing users to compile on demand should go away - a feature which |
That’s not how I read it. In “this feature will probably never go away,” “this feature” is |
imo usability and refactorability far outweigh any performance considerations. |
This was not my understanding of nodejs/ecmascript-modules#32 . Is it intended that a file could have a known format but not act like it has that format? |
@GeoffreyBooth |
just to be really clear about |
@devsnek and others who want this enabled by default, do you mind opening a new issue where you propose how it should work in our new implementation as things stand now? What would take precedence between There might very well be consensus for enabling extension searching via configuration, since it clearly will be possible via loaders (I think). So we might as well sketch out what that would look like, especially since configurability arguably improves the feature even if it’s on by default, since configuration improves performance by limiting the number of extensions that Node needs to try. |
@GeoffreyBooth i made an implementation locally where it goes mjs -> cjs -> the rest. i haven't made a pr yet because it doesn't include the mode flag, and i'd rather not put work in for something that's going to be replaced, so i'm waiting to see how i can layer this on the changes brad is making for loaders. As for the configuration, webpack has option to configure these extensions that also happens to be ordered, so i think we have something to work from there. Overall, i've never actually seen someone have an issue with the ordering, so i'm not that worried, but it would definitely make a good addition. people not using esm would benefit from that option too, so i don't think we need to tie it to our implementation. |
We didn't get time to discuss |
@devsnek I think the purpose of the configuration wouldn’t only be to specify the search order, but also to specify which extensions are searched; and which directory indexes are searched (if any). For example if a user has no @ljharb You’re welcome to open an issue to propose removing it from the new implementation. I’m sorry you didn’t realize it was part of the PR. The explanation for why we felt it was necessary is here. |
Proposing removal would be premature without at least openly discussing it in a single meeting, so I can try to understand why it's needed. That explanation, fwiw, doesn't seem to justify it when a more general "parse extension X as if it's extension Y" exists.
You can't import anything in CJS; only in ESM, and I don't know what a "commonJS package" is - "commonJS" and "ESM" are adjectives that describe a file, not a package. |
@ljharb Please open a new issue. |
This comment has been minimized.
This comment has been minimized.
IIRC the primary reason for a |
@GeoffreyBooth i'm agreed there should be lots of config here, i just don't really want to get into the design. i think some magic combo of loaders and copying from https://webpack.js.org/configuration/resolve/#resolveextensions will be enough. I'm just not sure what that is because we don't have loaders finalized yet. Even without the above, i think searching stands as a fantastic feature on its own and as there are no good technical arguments against turning it on by default[1] i'm hopeful we can move at a good pace. 1: in the current impl there is a chance of a performance hit if your dependencies use searching that hits a lot of extensions further down the list. This is possible to fix by performing all existence checks in parallel, but this has not been done yet. given that this is possible, i wouldn't consider performance a blocker. |
One other point: the extension-searching feature is also highly coupled with our strategy for resolving bare-specifier imports. If we use "index" or "main", as in the CJS implementation, then presumably we'd need to search over extensions in a like manner. "Deep package imports" are subject to the same considerations. On the other hand, if we are committed to not searching over extensions, then that seems to imply that we will need a different mechanism (e.g. the exports map proposal or something else) for resolving bare imports or deep bare imports. |
Resolved per nodejs/ecmascript-modules#48 |
It looks like per nodejs/modules#268 that node's resolution for modules requires a file extension by default. Typescript wants this to be included by adding "js" to files that are imported.
The topic of file extension and directory index resolution (whether automatic or opt-in) is on the agenda for this week, but I can’t seem to find an issue where it has been discussed on its own. It’s not one of the features on the list in our README, though it was mentioned in our road map as being a feature removed as part of Phase 1. It was mentioned by @devsnek and @ljharb in #253 as features that they would want before the new implementation is released. I thought this topic should get its own issue so that the broader group’s attention is drawn to it, since the entire group is significantly larger than the people who attend the meetings.
I’m going to try to keep this post neutral, and I’ll update this top post as needed when people point out things I haven’t thought of.
What file extension/directory index resolution is
In CommonJS, it’s idiomatic to write code like this:
Where
./startup
is resolved as either./startup.js
or./startup/index.js
, or potentially other file extensions.Why users like it
Aside from saving users from typing three or nine characters, there’s a big usability benefit in saving refactoring effort when a file grows to the point where it needs to be refactored from one into several. In the example above, your project could start with a
startup.js
file and when it grows unwieldy you could refactor intostartup/index.js
which in turnrequire
s several other files, and you wouldn’t need to update any of the files that alreadyrequire('./startup')
.What we need to decide
Since we’re planning on building loaders, and since it seems fair to assume that loaders will be capable of rewriting specifiers, file extension/directory index resolution will be possible one way or another in ESM. So the question isn’t whether it should be possible, but rather how easy it should be: enabled by default, enabled via opt-in
package.json
configuration, or enabled via a package-level loader. Or looked at another way, do we want to encourage the practice, slightly discourage it, or strongly discourage it? The arguments for not enabling it automatically are usually the same, so I’m grouping those together.Arguments for supporting file extension/directory index resolution automatically
This is already supported automatically by
require
in CommonJS. That’s what users are familiar with.It’s idiomatic to leave off extensions in CommonJS; actually typing them is unusual.
It’s also idiomatic to leave off extensions in ESM JavaScript written to be transpiled by Babel or
esm
. There are countless projects out there written in ESM syntax that assume automatic file extension/directory index resolution, and many of those projects might very well work out-of-the-box if we continued to support automatic file extension/directory index resolution.Arguments against supporting file extension/directory index resolution automatically
Browsers don’t automatically resolve file extensions or directory index files (unless a server is explicitly configured to do so, but this is practically unheard of), and we’re aiming to be as equivalent with browsers as possible. As the import maps proposal puts it:
By making file extension/directory index resolution opt-in, we encourage people to write packages that are compatible with browsers by default. This is especially important because leaving off the extension is currently idiomatic; we’re encouraging the establishment of a new idiom, where code is idiomatically capable of running in either environment (at least, with regards to this; obviously there are lots of other ways people can create packages that are incapable of running in browsers).
Node may someday support
import
of network path URLs, likeimport Three from 'https://unpkg.com/three@0.102.1/src/Three.js'
. Presumably extension searching will not apply to such URLs. It would be inconsistent for automatic file extension/directory index resolution to work forfile://
URLs but nothttps://
URLs. Such a system would mean that a package relying on automatic file extension/directory index resolution would work when installed locally but fail when loaded from a network URL.There’s a performance cost to automatic file extension/directory index resolution, that continues to grow as more supported file extensions are added. It’s one thing to search for
startup.js
, and then when it’s not found to search forstartup/index.js
; it’s another to search forstartup.js
,startup.mjs
,startup.wasm
(etc.) and thenstartup/index.js
,startup/index.mjs
,startup/index.wasm
etc. This performance hit is a big reason thatrequire.extensions
was deprecated. From Node’s docs:By requiring this to be opt-in via configuration or a loader, that configuration or loader can specify how this feature should behave. For example, maybe
.mjs
is the only supported extension automatically resolved, and the performance cost is reduced.The UX becomes more complicated now that we have multiple supported extensions. When a folder contains
file.js
,file.mjs
andfile.wasm
and youimport ‘./file’
, which file gets loaded? When support for new extensions is added to core, do they always get added last in precedence?We can release our new implementation unflagged without automatic file extension/directory index resolution and choose to add it later, but we can’t do the reverse.
Arguments for supporting it via package-level configuration
As part of Node proper, performance would be as optimized as it could be.
This would standardize users’ use of file extension/directory index resolution in a way that supporting it via third-party loaders would not.
Arguments for supporting it via a package-level loader but not via configuration
If people offer new arguments (or correct these) I’ll update this post.
Background: https://youtu.be/M3BM9TB-8yA?t=835
The text was updated successfully, but these errors were encountered: