--entry-type=auto #55

GeoffreyBooth · 2019-03-09T01:28:21Z

This implements --entry-type=auto per the entry points proposal.

For potentially ambiguous initial entry points (.js or extensionless files in a package scope with no "type" field, string input via --eval or --print or STDIN), --entry-type=auto tells Node to tokenize the source code to look for import or export statements. If any are found, the initial entry point is treated as if the user had specified --entry-type=module; otherwise like --entry-type=commonjs.

I’m using the Acorn tokenizer because its full parser lacks support for import() expressions. The tokenizer is also faster, and can exit early as soon as the first import or export statement is found. Acorn is already a part of the Node codebase, and I’m not loading it unless --entry-type=auto is specified.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

devsnek

"In the face of ambiguity, refuse the temptation to guess."

- The Zen of Python

devsnek · 2019-03-09T01:52:50Z

lib/internal/modules/esm/detect_type.js

+      prevToken = token;
+    }
+  } catch {
+    return 'commonjs';


garbage in shouldn't be 'commonjs' out. this should be an error saying node couldn't guess the parse goal.

So if this catch ever gets reached, it’s because the tokenizer fails to parse the syntax. I read the source code for Acorn’s tokenizer and as far as I can tell, the only exceptions it throws are for unterminated strings and template literals.

I can throw an error here, but it would be pretty generic, something like “Could not parse input source code.” Whereas if I let Node parse and evaluate the code, it would give a specific error with the line number and details of the syntax error. Either way the user is getting an error message; but if we allow this to hand off to the CommonJS loader, the user gets a better error.

How about this: 4d841b3

src/node_options.cc

ljharb

Concur with @devsnek’s review here. If we can’t detect the type unambiguously from syntax, it should throw.

What happens if i specify -a along with --type=module, or -a along with -type of any value? I’d expect an error.

doc/api/cli.md

lib/internal/errors.js

doc/api/cli.md

lib/internal/modules/esm/default_resolve.js

test/es-module/test-esm-type-auto.js

ljharb · 2019-03-09T06:45:23Z

Repeating so it doesn't get lost: What happens if i specify -a along with --type=module, or -a along with -type of any value? I’d expect an error.

GeoffreyBooth · 2019-03-09T07:28:30Z

Repeating so it doesn’t get lost: What happens if i specify -a along with –type=module, or -a along with -type of any value? I’d expect an error.

I would too. This is actually a bug in the implementation in general right now, not specific to this PR; you can pass -m and --type=commonjs together, or --type=module and --type=commonjs together, and no error is thrown. Do you mind opening an issue?

ljharb · 2019-03-09T08:05:43Z

#56

lib/internal/modules/esm/detect_type.js

GeoffreyBooth · 2019-03-09T20:50:58Z

@ljharb and @devsnek I think I’ve addressed all your notes, aside from -a. Is there anything else?

I can split off -a into a separate PR. That way, this can land without it and then -a can be debated separately. Would that be acceptable?

…ins import or export

…ens approach was too naive

… more tests

Co-Authored-By: GeoffreyBooth <[email protected]>

GeoffreyBooth added the enhancement New feature or request label Mar 9, 2019

devsnek suggested changes Mar 9, 2019

View reviewed changes

devsnek reviewed Mar 9, 2019

View reviewed changes

src/node_options.cc Outdated Show resolved Hide resolved

ljharb suggested changes Mar 9, 2019

View reviewed changes

GeoffreyBooth force-pushed the type-auto branch from adb1259 to f6d2ce9 Compare March 9, 2019 05:57

ljharb mentioned this pull request Mar 9, 2019

Throw error when conflicting type params are passed #56

Closed

ljharb reviewed Mar 9, 2019

View reviewed changes

lib/internal/modules/esm/detect_type.js Outdated Show resolved Hide resolved

GeoffreyBooth force-pushed the type-auto branch from 1125984 to 9f42d1b Compare March 9, 2019 08:27

ljharb mentioned this pull request Mar 10, 2019

esm: scoped --type, cpp refactoring #57

Closed

MylesBorins force-pushed the modules-lkgr branch from bec588f to ea59221 Compare March 11, 2019 22:56

nodejs-ci force-pushed the modules-lkgr branch 3 times, most recently from 9301a06 to e721cd2 Compare March 14, 2019 08:10

MylesBorins force-pushed the modules-lkgr branch 2 times, most recently from 484d1fb to 7efc53d Compare March 18, 2019 22:07

nodejs-ci force-pushed the modules-lkgr branch 3 times, most recently from c7fa700 to d69f765 Compare March 21, 2019 08:06

MylesBorins force-pushed the modules-lkgr branch 5 times, most recently from 335d584 to 9a343ce Compare March 21, 2019 19:09

nodejs-ci force-pushed the modules-lkgr branch 2 times, most recently from 871b78b to 3a00b51 Compare March 23, 2019 08:06

nodejs-ci force-pushed the modules-lkgr branch from 3a00b51 to bc101f6 Compare March 24, 2019 08:06

MylesBorins force-pushed the modules-lkgr branch 3 times, most recently from fd5b55a to 3a40599 Compare March 27, 2019 02:42

nodejs-ci force-pushed the modules-lkgr branch from 3a40599 to 6fe40a4 Compare March 27, 2019 08:06

MylesBorins force-pushed the modules-lkgr branch from 6fe40a4 to d9cdfd8 Compare March 27, 2019 18:53

nodejs-ci force-pushed the modules-lkgr branch from d9cdfd8 to 230f1f2 Compare March 28, 2019 08:06

GeoffreyBooth and others added 10 commits April 2, 2019 23:26

esm: --type=auto, to detect module type based on whether source conta…

498632f

…ins import or export

esm: refactor import/export detection to use full parsing, as the tok…

51b1b74

…ens approach was too naive

esm: tests for --type=auto

d42bbd4

esm: acorn doesn't support import(), so revert to tokenizer approach;…

706a6c5

… more tests

esm: simplify logic in detect loop

7353702

doc: --entry-type=auto

3240a61

esm: fixup: code style

beda225

esm: --entry-type=auto should throw syntax errors directly

88d44c8

esm: fixup: code style

50437ce

Co-Authored-By: GeoffreyBooth <[email protected]>

esm: fix tests

6bb7e1a

GeoffreyBooth force-pushed the type-auto branch from 9f42d1b to 6bb7e1a Compare April 3, 2019 07:00

GeoffreyBooth changed the title ~~--type=auto~~ --entry-type=auto Apr 4, 2019

GeoffreyBooth mentioned this pull request Apr 4, 2019

Replace --entry-type with --input-type, plus --input-type=auto #68

Closed

GeoffreyBooth closed this Apr 18, 2019

ljharb deleted the type-auto branch April 18, 2019 08:10

GeoffreyBooth mentioned this pull request Aug 19, 2021

Detect require vs. import based on the entry point, not package.json nodejs/node#39353

Closed

GeoffreyBooth mentioned this pull request Oct 4, 2023

Proposal: Set --experimental-default-type mode by detecting ESM syntax in entry point nodejs/node#50043

Closed

Uh oh!

Uh oh!

--entry-type=auto #55

--entry-type=auto #55

Uh oh!

Conversation

GeoffreyBooth commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

devsnek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsnek Mar 9, 2019

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Mar 9, 2019

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Mar 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ljharb commented Mar 9, 2019

Uh oh!

GeoffreyBooth commented Mar 9, 2019

Uh oh!

ljharb commented Mar 9, 2019

Uh oh!

Uh oh!

GeoffreyBooth commented Mar 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GeoffreyBooth commented Mar 9, 2019 •

edited

Loading

devsnek left a comment •

edited

Loading