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

esm: scoped --type, cpp refactoring #57

Open
wants to merge 37 commits into
base: modules-lkgr
from

Conversation

Projects
None yet
7 participants
@guybedford
Copy link
Contributor

guybedford commented Mar 10, 2019

With the current implementation, there is an edge case of --type that doesn't quite work out.

Consider:

main.js

import './dep.js';

where dep.js is also an ECMAScript module.

Currently, node --type=module main.js will execute main.js as an ECMAScript module, but then when it loads dep.js it forgets the type flag and goes back to the old rules of CJS treatment, which is usually not the intention.

This PR fixes this case by having the top-level --type flag apply as a scope on the folder of the entry point.

I ended up refactoring the type errors and ESM_FORMAT implementation in the process, making both more consistent as a result while retaining the same implementation behaviours as previously. Moving ESM_FORMAT back into the C++ code in the process also fixes the TODO on the package.json cache sharing issue.

I've also updated the spec on how the --type flag works to reflect the above, and also fixed some spec bugs where it wasn't quite aligning with our implementation in that files without an extension that weren't the main were still being permitted.

guybedford and others added some commits Aug 28, 2018

esm: remove .json support
Refs: nodejs/modules#180

PR-URL: #6
Reviewed-By: Myles Borins <MylesBorins@google.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
esm: remove .node support
Refs: nodejs/modules#180

PR-URL: #6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
esm: remove .js support
Refs: nodejs/modules#180

PR-URL: #6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
esm: remove node specifier resolution algorithm
Refs: nodejs/modules#180

PR-URL: #6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
doc: document minimal kernel
PR-URL: #6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
esm: Remove --loader.
PR-URL: #6
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Myles Borins <mylesborins@google.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Mar 10, 2019

To me, it was always clear that dependencies would be uneffected by the type flag, and I’m pretty sure we did discuss it.

Dependencies that have their own scopes, like subfolders under node_modules that have their own package.json files, are unaffected by the --type flag. But files in the same scope of the initial entry point need to be treated the same way as the initial entry point. Otherwise we have the case where, to use the example above, main.js is treated as ESM but dep.js right next to it in the same folder is treated as CommonJS. It’s never been the intent for side-by-side .js files to be treated as different types. That’s a bug.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Mar 10, 2019

@GeoffreyBooth then we need to revisit --type at all, i guess. All files brought in by import or require should behave the same, unrelated to any flags passed to the process. The only reason imo for allowing --type in the first place is because entry points without a package.json, or extensionless files, lack the ability to otherwise provide an explicit signal.

In other words, in that example, main.js is ESM because of the flag, but dep.js is CJS. It'd need to be dep.mjs, or it'd need to have a package.json somehow overriding the CJS parse goal for dep.js, for it to be interpreted differently.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Mar 10, 2019

--type only applies to .js files where there’s no package.json "type" field (or the non-file cases like --eval). We had a long discussion about all the errors that should be thrown when --type conflicts with the "type" field or with explicit file extensions .mjs or .cjs. So --type only applies to .js files in a scope with no "type" field, or the non-file cases.

In the example above, if there was a defined "type" field either way, there would be no issue. A "type": "module" would cause both files to be interpreted as ESM, and they’d run fine; or a "type": "commonjs" would cause the command node --type=module main.js to throw an error, as the --type flag would be in conflict with the "type" field. The --type flag is the third order of priority for determining the format, after the file extension and "type" field. If either of the other two are explicit, --type never applies (or causes an error to be thrown).

This was discussed most explicitly regarding --type=auto, which by definition can’t be used with explicit extensions or a scope with a "type" field (since there’d be no need for detection in those cases). The only time a user can use auto is a project with .js files and no "type" field, and it wouldn’t make sense for --type=auto to potentially tell Node to handle the initial entry point as ESM but then all the other files in the project are treated as CommonJS. The whole purpose of --type=auto is to set the scope based on what type it detects from the initial entry point.

This is just a bugfix. There’s never been a proposal or a PR where it was intended for .js files in the same scope to not be treated as the same type. The docs for --type don’t say “use --type=module file.js to run file.js as an ES module, but if file.js imports any other files, you also need to set "type": "module in the nearest parent package.json.” If setting "type" were required for any project with more than one file, then --type would have almost no purpose; it would only apply to non-file cases and single-file projects. That’s never been the intent.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Mar 10, 2019

it's been my consistent understanding that --type was only meant to apply to the entry file. i think we need to discuss this further.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Mar 10, 2019

Again, it’s a type, not a mode. It should not persist beyond parsing the entry point.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Mar 10, 2019

@GeoffreyBooth my comment here was me asking about this very thing, and your reply said specifically it was just for the entry point.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Mar 11, 2019

@GeoffreyBooth my comment here was me asking about this very thing, and your reply said specifically it was just for the entry point.

Huh? You were asking what the isMain variable in the --type=auto code meant, and I said it referred to the initial entry point, because that’s what the detection is being run on (and that only). As we’ve discussed in depth, --type=auto doesn’t run detection on every file in a project, because there could be many files in a project that lack import or export but were still intended to be treated as ESM. We discussed how --type=auto was meant to use the presence of import or export in the initial entry point to define how to treat the project. --type=auto would be nearly useless if it were only usable on single-file projects, and we discussed its use on multi-file projects.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Mar 11, 2019

The intent is to use it in one-off scripts. Projects have a package.json, and don’t need a flag on the CLI.

@MylesBorins MylesBorins force-pushed the nodejs:modules-lkgr branch from bec588f to ea59221 Mar 11, 2019

@nodejs-ci nodejs-ci force-pushed the nodejs:modules-lkgr branch from ea59221 to c6d1b01 Mar 12, 2019

ext === '.mjs' && asyncESM.typeFlag === 'commonjs') {
throw new ERR_TYPE_MISMATCH(
entryPath, ext, asyncESM.typeFlag, 'extension');
}

This comment has been minimized.

@jdalton

jdalton Mar 13, 2019

Member

It seems valid that a .cjs entry module could load a .js file with dynamic import() and it be treated as ESM with the --type=module. These checks would prevent that wouldn't they?

This comment has been minimized.

@guybedford

guybedford Mar 13, 2019

Author Contributor

No, the main in this context means these checks are only done for the main entry point into Node.js, not for any top-level resolution.

This comment has been minimized.

@jdalton

jdalton Mar 13, 2019

Member

Right, the checks are done for the main which is the scenario I was describing. There is no reason to stop a .cjs file from being the main script when --type=module.

This comment has been minimized.

@guybedford

guybedford Mar 13, 2019

Author Contributor

In this model of the --type being a scope, that is something I could see, yes.

But if this doesn't land, and we stick with --type being a file indicator, the check stays.

(Note that this PR doesn't add these validations, it just refactors them)

This comment has been minimized.

@jdalton

jdalton Mar 13, 2019

Member

As part of this PR though I think those guards should be revised/removed as appropriate for the scenario.

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Mar 13, 2019

I think I am in favor of less "virtual package json" magic, so... -0

@nodejs-ci nodejs-ci force-pushed the nodejs:modules-lkgr branch from 9301a06 to e721cd2 Mar 14, 2019

@MylesBorins MylesBorins force-pushed the nodejs:modules-lkgr branch from 3e26030 to 484d1fb Mar 18, 2019

@MylesBorins MylesBorins force-pushed the nodejs:modules-lkgr branch from 484d1fb to 7efc53d Mar 18, 2019

@nodejs-ci nodejs-ci force-pushed the nodejs:modules-lkgr branch 3 times, most recently from c7fa700 to d69f765 Mar 19, 2019

@MylesBorins MylesBorins force-pushed the nodejs:modules-lkgr branch 5 times, most recently from 335d584 to 9a343ce Mar 21, 2019

@nodejs-ci nodejs-ci force-pushed the nodejs:modules-lkgr branch 3 times, most recently from 3a00b51 to bc101f6 Mar 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.