Skip to content
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

Trailing slashes #244

Closed
domenic opened this issue Jan 22, 2021 · 10 comments
Closed

Trailing slashes #244

domenic opened this issue Jan 22, 2021 · 10 comments

Comments

@domenic
Copy link
Collaborator

domenic commented Jan 22, 2021

@RReverser pointed out to me today that in Node.js, you can do either require("lodash") or require("lodash/"), and both will map to the lodash package's main module.

Similarly, in Node.js you can do either require("./directory") or require("./directory/"), and this will look for the "main" field in ./directory/package.json and require that module.

With import maps, these trailing slash variants are not possible to remap in the same way. We currently require trailing slashes to match, so you can do "lodash/": "/node_modules/lodash/" and "./directory/": "./some/other/directory/". But then, importing those will give URLs like https://example.com/node_modules/lodash/, which is not what you want; you wanted the URL to be https://example.com/node_modules/lodash/main.mjs.

Another aspect to consider is if we introduce import.meta.resolve(). What should the result of import.meta.resolve("lodash/fp/..") be? As currently proposed, it would be whatever URL "lodash/" maps to. Which, per the above, is not so useful.


So the first question is, do we care about this case? I'm inclined to take a wait-and-see approach. In particular, my initial instinct is that even though Node has two ways to require the same module, "lodash" and "lodash/", maybe it's OK that on the web only the first way works. I'd be open to changing this opinion as we see more people running into issues in the wild.

Then, if we did want to address this, how would we? Simply lifting the "slashes must match" restriction is not great, because what if you want both "lodash/" -> </node_modules/lodash/main.mjs> and "lodash/foo.mjs" -> </node_modules/lodash/foo.mjs>? You wouldn't be able to get both.

One route for addressing this, which is attractive to me, might be to try to canonicalize all trailing slashes in import specifiers away. That is, "lodash/" literally means the same thing as "lodash". This is trickier for cases like "/directory/"; it'd be a breaking change (but maybe a web-compatible one?) to always treat that the same as "/directory", i.e. change it to by default request the URL </directory> instead of the URL </directory/>. Maybe we could only impose the normalization when an import map gets involved? But that might be a bit messy.

@ljharb
Copy link

ljharb commented Jan 22, 2021

Once difference to note is that when that package has an "exports" field, the / form no longer works (unless it has "./": "./", which is deprecated).

However, canonicalizing trailing slashes still seems like the proper thing to do.

@guybedford
Copy link
Collaborator

@RReverser pointed out to me today that in Node.js, you can do either require("lodash") or require("lodash/"), and both will map to the lodash package's main module.

This is no longer the case for the ES modules implementation of Node.js, and a special ERR_UNSUPPORTED_DIR_IMPORT error is thrown instead.

This has been part of the effort of deprecating some of the magic around module resolution by not including support for directory resolution in the ES modules resolver implementation to try create alignment with browser resolution.

There is a lot to this conversation though, happy to flesh out any aspects further as necessary.

@RReverser
Copy link

@guybedford I think it's slightly different in that ESM in Node.js doesn't support directory imports altogether anymore, regardless of whether it was specified with a trailing slash?

@guybedford
Copy link
Collaborator

@RReverser yes you are correct this is based on strictly checking if the file is a directory not the trailing slash. So in Node.js a trailing slash that is not a directory would throw MODULE_NOT_FOUND instead.

But you can still resolve a directory import for the main for CJS, eg via import('pkg') where pkg has a "main": "./dir/" field will work fine. This was necessary for backwards compatibility in loading CJS packages.

There is a separate deprecation PR currently in progress to deprecate this behaviour for ES modules though as well, in ensuring "main" always points to a complete path in the page.

Regardless, Node.js fully deprecates the resolution of trailing slashes, which is the important point to note here. They simply can't resolve in Node.js anymore.

@ljharb
Copy link

ljharb commented Jan 23, 2021

This isn’t accurate - node has not in any way deprecated trailing slashes in CJS, for packages that lack an “exports” field. That may not be relevant to import maps, though, which are for ESM.

@guybedford
Copy link
Collaborator

@ljharb import and import() do not support resolving directories with trailing slashes at all. There is no specifier with a trailing slash that will resolve through these mechanisms without customizing the resolve hook.

@ljharb
Copy link

ljharb commented Jan 23, 2021

@guybedford agreed, but "fully deprecated" implies "with require" as well, so I wanted to clarify.

@RReverser
Copy link

RReverser commented Jan 23, 2021

agreed, but "fully deprecated" implies "with require" as well, so I wanted to clarify.

Also probably worth mentioning node --es-module-specifier-resolution=node which makes them work as well.

But yeah, to be honest, I didn't know Node chose not to support directory -> main file imports by default in ESM syntax, not even via exports field. This seems unfortunate as it creates inconsistency between what you can do with npm packages vs local dirs.

@domenic
Copy link
Collaborator Author

domenic commented Oct 14, 2021

I think I'm OK not supporting this, but if we have people using import maps in the wild where this is an issue, please let me know (preferably with a link to the site).

@domenic domenic closed this as completed Oct 14, 2021
@guybedford
Copy link
Collaborator

@domenic FWIW the deprecation path is moving now in Node.js to fully disable trailing slash imports for ESM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants