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

treat extensionless as .js #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

treat extensionless as .js #1

wants to merge 4 commits into from

Conversation

srolel
Copy link
Owner

@srolel srolel commented Jun 28, 2020

@guybedford

These two separate commits address the issue of using extensionless modules:

34a0f30 - extensionless files will be treated as .js
69a29ee - using the --loader flag will not force using ESM loader

@guybedford
Copy link

So this works but the problem then specifically is that it will support eg:

import wasm from './x.wasm'

being loaded as an ES module! Which no one will be able to support to land.

The next logical step is to explicitly do this only for a file without any extension (path.extname(file) === '').

That is something I also never considered as a possibility, because it doesn't ambiguate from files that just use . in their naming (component.main), but perhaps that might be a suitable alternative to try as a first Node.js PR approach.

@@ -21,9 +21,6 @@ function resolveMainPath(main) {
}

function shouldUseESMLoader(mainPath) {
const userLoader = getOptionValue('--experimental-loader');
if (userLoader)
return true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the --loader flag absolutely must enforce the ESM loader, because otherwise the loader will not be applied at all, when it should definitely be used.

Fixing the extension issue on its own should be enough to support the use case of loaders resolving extensionless files though I believe?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes I was expecting this might be the case. And yeah, the first commit by itself should take care of it.

I was going off of this from the issue:

./entrypoint is executed as a CommonJS module whether or not --loader is passed.

It's a little unclear to me where the boundary between the ESM and CJS loaders is, as the ESM loader still seems to be able to load CommonJS files.

@srolel
Copy link
Owner Author

srolel commented Jun 29, 2020

So this works but the problem then specifically is that it will support eg:

import wasm from './x.wasm'

being loaded as an ES module! Which no one will be able to support to land.

The next logical step is to explicitly do this only for a file without any extension (path.extname(file) === '').

That is something I also never considered as a possibility, because it doesn't ambiguate from files that just use . in their naming (component.main), but perhaps that might be a suitable alternative to try as a first Node.js PR approach.

I'm not sure I follow, why would x.wasm be loaded as an ES module there? it does only check for ext === '' in that commit.

I've added another commit that treats any unknown extensions the same way as .js, with the exception thrown only in specific, prohibited cases (such as .wasm without the flag). but that might be overreaching for a first PR.

@guybedford
Copy link

Ah yeah sorry I missed that you are explicitly handling the no extension case as I mentioned at the end there.

Ok this seems good to me then actually... do you want to try a PR?

Note if there is pushback, the main-only constraint was my initial suggestion but let's see how it goes.

@srolel
Copy link
Owner Author

srolel commented Jun 29, 2020

Ah yeah sorry I missed that you are explicitly handling the no extension case as I mentioned at the end there.

Ok this seems good to me then actually... do you want to try a PR?

Note if there is pushback, the main-only constraint was my initial suggestion but let's see how it goes.

Ah, nice! I actually added that other commit just now, GH seems a little flaky atm and it failed before. It will handle unknown extensions as well, but throw if .json or .wasm are loaded without the flag. But I can definitely start with just the first commit.

Copy link

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest keeping what you had originally here and just running a PR against Node.js core with a careful description and link to the corresponding issues that it solves.

Explicitly unsupported extensions is not really a scalable approach for Node.js given that the project doesn't necessarily know what extensions might need to be supported in future. This is the standard case of "correctness" trumps "practicality", but unfortunately that is just the calculus.

'.mjs': 'module'
'.mjs': 'module',
'.json': 'unsupported',
'.wasm': 'unsupported'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't do these explicit opt-outs - the problem is that we can't predict what file extensions might need to be supported by the module system in future.

@srolel
Copy link
Owner Author

srolel commented Jun 29, 2020

I would suggest keeping what you had originally here and just running a PR against Node.js core with a careful description and link to the corresponding issues that it solves.

Explicitly unsupported extensions is not really a scalable approach for Node.js given that the project doesn't necessarily know what extensions might need to be supported in future. This is the standard case of "correctness" trumps "practicality", but unfortunately that is just the calculus.

Got it. I'll work on a PR later today. This was an attempt to support the use-case you mentioned (periods used in file names without having a "real" extension). I don't see another way to do that other than replace opt-in with opt-out, but that should be a pretty obscure use-case.

@guybedford
Copy link

Ok let me know if you want a final review before posting up to core.

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

Successfully merging this pull request may close these issues.

2 participants