Warn about `--type` with shebang #37

jkrems · 2019-02-16T21:20:16Z

Follow-up to #35

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

Refs: nodejs/modules#180 PR-URL: #6 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Refs: nodejs/modules#180 PR-URL: #6 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: #6 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: #6 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR URL: #12

MylesBorins · 2019-02-16T21:35:45Z

@jkrems can you please rebase

Follow-up to #35

jkrems · 2019-02-16T21:38:08Z

Applied on top of latest lkgr.

devsnek · 2019-02-16T21:55:40Z

i'd word this a little bit stronger. using arguments in hashbangs is something that should be avoided in general.

ljharb · 2019-02-16T22:29:51Z

doc/api/cli.md

-Works with stdin, `--eval`, `--print` as well as standard execution.
+Works with stdin, `--eval`, `--print` as well as standard execution
+when envoking `node` explicitly.
+Does not work reliably when running via shebang (`#!/usr/bin/env node`)


should perhaps (like io.js provided both iojs and node binaries) node provide a node-esm or similar binary, so they can use that in the shebang instead?

Yeah, I assume that'll be where it's heading. The question would be: Would this make a better general command / should it replace node as the "default" binary people would run?

Only if they're invoking node on an extensionless ESM file - if the file has an extension, or if it's CJS, then node would still be preferred.

Since package.json bins can have extensions, this really only affects executable one-off ESM scripts, which won't be the majority (but are still an important use case)

Right, but if node-esm always works (after porting your code to ESM) and node works most of the time - why would you ever tempt fate and use node?

i wouldn't bet on another binary for this --mode option, given opposition to the design of the option, and the weight of an extra binary for very little configuration.

@jkrems it wouldn't work on CJS files, and i'd expect it to throw if you tried to run with --type=esm on a file that had a non-esm file extension - the better question would be why would you tempt fate and use the alternative :-)

How about something like:

Suggested change

Does not work reliably when running via shebang (`#!/usr/bin/env node`)

Note that this flag is not a reliable solution for shebang scripts (`#!/usr/bin/env node -m`) due to multiple arguments not being supported in POSIX environments.

it'd be anything that respects exec(3), so most POSIX platforms

Sure, updated to posix.

Feel free to update the commit directly. This PR is just me suggesting a fix to an earlier PR, I don't have any personal attachment to the exact wording. :)

GeoffreyBooth · 2019-04-18T04:36:02Z

I think this is no longer relevant now that we no longer have a flag that applies to files?

jkrems · 2019-04-18T16:11:43Z

Correct, closing! :)

guybedford and others added 11 commits February 16, 2019 13:48

esm: remove .json support

91bfcce

Refs: nodejs/modules#180 PR-URL: #6 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

esm: remove .node support

a36048c

Refs: nodejs/modules#180 PR-URL: #6 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

esm: remove .js support

5f0de42

Refs: nodejs/modules#180 PR-URL: #6 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

esm: remove node specifier resolution algorithm

52f316a

Refs: nodejs/modules#180 PR-URL: #6 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

doc: document minimal kernel

35461ad

PR-URL: #6 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

esm: Remove --loader.

ed6814e

PR-URL: #6 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

esm: minimal resolver spec and implementation refinements

12f1e8d

PR URL: #12

specify import file specifier resolution proposal

7d5e258

irp type implementation

0fb0529

esm: top-level --type, -m flags, spec updates

557490b

esm: implement top-level --type flag, type:esm -> type:module

968c8f1

jkrems mentioned this pull request Feb 16, 2019

Modules lkgr rebase no loader #35

Closed

MylesBorins force-pushed the modules-lkgr branch from 4514848 to 968c8f1 Compare February 16, 2019 21:34

Warn about --type with shebang

12905b0

Follow-up to #35

jkrems force-pushed the jk-mention-shebang branch from 744065f to 12905b0 Compare February 16, 2019 21:37

ljharb reviewed Feb 16, 2019

View reviewed changes

nodejs-ci force-pushed the modules-lkgr branch 2 times, most recently from 0dbed6b to 621a632 Compare February 18, 2019 09:06

GeoffreyBooth added the modules-agenda label Feb 19, 2019

nodejs-ci force-pushed the modules-lkgr branch from 621a632 to d89e9ee Compare February 19, 2019 09:07

MylesBorins force-pushed the modules-lkgr branch from d89e9ee to 22339db Compare February 22, 2019 18:03

nodejs-ci force-pushed the modules-lkgr branch 4 times, most recently from b84c079 to a4955e2 Compare February 26, 2019 09:06

MylesBorins mentioned this pull request Feb 26, 2019

Node.js Foundation Modules Team Meeting 2019-02-27 nodejs/modules#276

Closed

nodejs-ci force-pushed the modules-lkgr branch from a4955e2 to 7c92011 Compare February 27, 2019 09:07

nodejs-ci force-pushed the modules-lkgr branch from ea59221 to c6d1b01 Compare March 12, 2019 08:06

MylesBorins mentioned this pull request Mar 13, 2019

Node.js Foundation Modules Team Meeting 2019-03-13 nodejs/modules#292

Closed

nodejs-ci force-pushed the modules-lkgr branch 2 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 3 times, most recently 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 mentioned this pull request Mar 27, 2019

Node.js Foundation Modules Team Meeting 2019-03-27 nodejs/modules#302

Closed

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 mentioned this pull request Apr 2, 2019

Replace --entry-type with --input-type #66

Closed

MylesBorins mentioned this pull request Apr 10, 2019

Node.js Foundation Modules Team Meeting 2019-04-10 nodejs/modules#310

Closed

jkrems closed this Apr 18, 2019

jkrems deleted the jk-mention-shebang branch April 18, 2019 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn about `--type` with shebang #37

Warn about `--type` with shebang #37

jkrems commented Feb 16, 2019

MylesBorins commented Feb 16, 2019

jkrems commented Feb 16, 2019

devsnek commented Feb 16, 2019

ljharb Feb 16, 2019

jkrems Feb 16, 2019

ljharb Feb 16, 2019

jkrems Feb 16, 2019

devsnek Feb 16, 2019

ljharb Feb 16, 2019

guybedford Mar 7, 2019 •

edited

Loading

devsnek Mar 7, 2019

guybedford Mar 7, 2019

jkrems Mar 7, 2019

GeoffreyBooth commented Apr 18, 2019

jkrems commented Apr 18, 2019

	Does not work reliably when running via shebang (`#!/usr/bin/env node`)
	Note that this flag is not a reliable solution for shebang scripts (`#!/usr/bin/env node -m`) due to multiple arguments not being supported in POSIX environments.

Warn about --type with shebang #37

Warn about --type with shebang #37

Conversation

jkrems commented Feb 16, 2019

Checklist

MylesBorins commented Feb 16, 2019

jkrems commented Feb 16, 2019

devsnek commented Feb 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth commented Apr 18, 2019

jkrems commented Apr 18, 2019

Warn about `--type` with shebang #37

Warn about `--type` with shebang #37

guybedford Mar 7, 2019 •

edited

Loading