Skip to content
This repository has been archived by the owner on Apr 16, 2020. It is now read-only.

Warn about --type with shebang #37

Closed
wants to merge 12 commits into from
Closed

Warn about --type with shebang #37

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
91bfcce
esm: remove .json support
guybedford Aug 28, 2018
a36048c
esm: remove .node support
MylesBorins Sep 12, 2018
5f0de42
esm: remove .js support
MylesBorins Oct 2, 2018
52f316a
esm: remove node specifier resolution algorithm
MylesBorins Oct 2, 2018
35461ad
doc: document minimal kernel
MylesBorins Oct 2, 2018
ed6814e
esm: Remove --loader.
jdalton Oct 2, 2018
12f1e8d
esm: minimal resolver spec and implementation refinements
guybedford Oct 24, 2018
7d5e258
specify import file specifier resolution proposal
guybedford Nov 29, 2018
0fb0529
irp type implementation
guybedford Jan 24, 2019
557490b
esm: top-level --type, -m flags, spec updates
guybedford Feb 10, 2019
968c8f1
esm: implement top-level --type flag, type:esm -> type:module
guybedford Feb 11, 2019
12905b0
Warn about `--type` with shebang
Feb 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Warn about --type with shebang 
Follow-up to #35
  • Loading branch information
Jan Olaf Krems committed Feb 16, 2019
commit 12905b0b4435f59278ca8da5fa849019d59475de
5 changes: 4 additions & 1 deletion doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,10 @@ Track heap object allocations for heap snapshots.
When using `--experimental-modules`, this informs the module resolution type
to interpret the top-level entry into Node.js.

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`)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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 :-)

Copy link
Contributor

@guybedford guybedford Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, updated to posix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. :)

since arguments inside of the shebang aren't well supported across platforms.

Valid values are `"commonjs"` and `"module"`, where the default is to infer
from the file extension and package type boundary.
Expand Down