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.

Sign up for GitHub

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

Jump to bottom

esm: replace --entry-type with --input-type #27184

Closed
wants to merge 1 commit into from
Closed

esm: replace --entry-type with --input-type #27184

wants to merge 1 commit into from

Conversation

GeoffreyBooth
Copy link
Member

This is a change discussed and approved by the modules working group per our meeting today on 2019-04-10. We’re hoping that this change can be merged in before the release of Node 12.

Per nodejs/modules#300 (comment), this PR replaces --entry-type with --input-type, a flag just like --entry-type but only for --eval, --print and STDIN.

This way we still provide a way to use ESM in those non-file inputs, but we’re removing the footgun that is --entry-type in its current form. To use ESM in files, the files need to end in .mjs or be in a "type": "module" package scope.

Tests and docs updated.

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 11, 2019
@GeoffreyBooth GeoffreyBooth mentioned this pull request Apr 11, 2019
Closed
@GeoffreyBooth GeoffreyBooth force-pushed the flag-for-string-input-only branch from 8e92ab2 to a4047d3 Compare April 11, 2019 02:16
@GeoffreyBooth GeoffreyBooth mentioned this pull request Apr 11, 2019
Merged
jkrems
jkrems reviewed Apr 11, 2019
View reviewed changes
doc/api/errors.md
@@ -2223,6 +2219,18 @@ closed.
These errors have never been released, but had been present on master between
releases.

<a id="ERR_ENTRY_TYPE_MISMATCH"></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this left in on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see the section head:

These errors have never been released, but had been present on master between
releases.

jkrems
jkrems approved these changes Apr 11, 2019
View reviewed changes
Copy link
Contributor

@jkrems jkrems left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM once the entry type error in the docs is removed

targos
targos reviewed Apr 11, 2019
View reviewed changes
lib/internal/main/repl.js Outdated Show resolved Hide resolved
@devsnek devsnek added esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. labels Apr 11, 2019
@devsnek devsnek requested a review from vsemozhetbyt April 11, 2019 16:33
vsemozhetbyt
vsemozhetbyt reviewed Apr 11, 2019
View reviewed changes
doc/api/errors.md Outdated Show resolved Hide resolved
joyeecheung
joyeecheung approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code LGTM. I will defer the decision around feature change to the modules group, but looks like people welcome it there, and I personally like this approach better. Great work researching into the details! 👍

ljharb
ljharb approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

the modules group achieved consensus on this change, so we do support it

@GeoffreyBooth GeoffreyBooth force-pushed the flag-for-string-input-only branch from c73e3a6 to 560c745 Compare April 14, 2019 01:42
@guybedford guybedford mentioned this pull request Apr 14, 2019
Closed
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22450/

@GeoffreyBooth GeoffreyBooth force-pushed the flag-for-string-input-only branch 3 times, most recently from ab4712a to fb9d934 Compare April 15, 2019 21:22
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22474/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22496/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22497/

@MylesBorins MylesBorins force-pushed the flag-for-string-input-only branch from d2eaa1e to 3e037fa Compare April 16, 2019 15:44
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22499/

MylesBorins
MylesBorins approved these changes Apr 16, 2019
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Contributor

landed in 96e46d3

MylesBorins pushed a commit that referenced this pull request Apr 16, 2019
@GeoffreyBooth @MylesBorins
esm: replace --entry-type with --input-type
96e46d3 
New flag is for string input only

PR-URL: #27184
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@ljharb ljharb deleted the flag-for-string-input-only branch April 16, 2019 17:19
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
chjj added a commit to chjj/bthreads that referenced this pull request Apr 30, 2019
@chjj
process/flags: add --input-type flag. see nodejs/node#27184.
9ccdd87
chjj added a commit to bcoin-org/bmocha that referenced this pull request May 3, 2019
@chjj
bin/bmocha: add --input-type flag. see nodejs/node#27184.
d68a580
@dnalborczyk dnalborczyk mentioned this pull request May 13, 2019
Closed
4 tasks
@aduh95 aduh95 mentioned this pull request Feb 9, 2021
Merged
@benjamingr benjamingr mentioned this pull request Mar 20, 2021
Closed
@snyk-bot snyk-bot mentioned this pull request May 10, 2021
Open
@BitcoinOutput BitcoinOutput mentioned this pull request Jul 16, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@targos targos targos approved these changes

@jkrems jkrems jkrems approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@GeoffreyBooth @nodejs-github-bot @MylesBorins @ljharb @jkrems @targos @joyeecheung @BridgeAR @vsemozhetbyt @devsnek