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

Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ module.exports = {
{
files: [
'doc/api/esm.md',
'test/es-module/test-esm-type-flag.js',
'*.mjs',
'test/es-module/test-esm-example-loader.js',
],
parserOptions: { sourceType: 'module' },
},
Expand Down
24 changes: 15 additions & 9 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,13 +256,6 @@ default) is not firewall-protected.**

See the [debugging security implications][] section for more information.

### `--loader=file`
<!-- YAML
added: v9.0.0
-->

Specify the `file` of the custom [experimental ECMAScript Module][] loader.

### `--max-http-header-size=size`
<!-- YAML
added: v11.6.0
Expand Down Expand Up @@ -499,6 +492,21 @@ added: v2.4.0

Track heap object allocations for heap snapshots.

### `-m`, `--type=type`

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

`-m` is an alias for `--type=module`.

### `--use-bundled-ca`, `--use-openssl-ca`
<!-- YAML
added: v6.11.0
Expand Down Expand Up @@ -699,7 +707,6 @@ Node.js options that are allowed are:
- `--inspect`
- `--inspect-brk`
- `--inspect-port`
- `--loader`
- `--max-http-header-size`
- `--napi-modules`
- `--no-deprecation`
Expand Down Expand Up @@ -887,7 +894,6 @@ greater than `4` (its current default value). For more information, see the
[debugger]: debugger.html
[debugging security implications]: https://nodejs.org/en/docs/guides/debugging-getting-started/#security-implications
[emit_warning]: process.html#process_process_emitwarning_warning_type_code_ctor
[experimental ECMAScript Module]: esm.html#esm_loader_hooks
[libuv threadpool documentation]: http://docs.libuv.org/en/latest/threadpool.html
[remote code execution]: https://www.owasp.org/index.php/Code_Injection
[secureProtocol]: tls.html#tls_tls_createsecurecontext_options
35 changes: 24 additions & 11 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1447,26 +1447,19 @@ a `dynamicInstantiate` hook.
A `MessagePort` was found in the object passed to a `postMessage()` call,
but not provided in the `transferList` for that call.

<a id="ERR_MISSING_MODULE"></a>
### ERR_MISSING_MODULE

> Stability: 1 - Experimental

An [ES6 module][] could not be resolved.

<a id="ERR_MISSING_PLATFORM_FOR_WORKER"></a>
### ERR_MISSING_PLATFORM_FOR_WORKER

The V8 platform used by this instance of Node.js does not support creating
Workers. This is caused by lack of embedder support for Workers. In particular,
this error will not occur with standard builds of Node.js.

<a id="ERR_MODULE_RESOLUTION_LEGACY"></a>
### ERR_MODULE_RESOLUTION_LEGACY
<a id="ERR_MODULE_NOT_FOUND"></a>
### ERR_MODULE_NOT_FOUND

> Stability: 1 - Experimental

A failure occurred resolving imports in an [ES6 module][].
An [ESM module][] could not be resolved.

<a id="ERR_MULTIPLE_CALLBACK"></a>
### ERR_MULTIPLE_CALLBACK
Expand Down Expand Up @@ -2208,6 +2201,27 @@ A non-specific HTTP/2 error has occurred.
Used in the `repl` in case the old history file is used and an error occurred
while trying to read and parse it.

<a id="ERR_INVALID_REPL_TYPE"></a>
### ERR_INVALID_REPL_TYPE

> Stability: 1 - Experimental

The `--type=...` flag is not compatible with the Node.js REPL.

<a id="ERR_INVALID_TYPE_EXTENSION"></a>
### ERR_INVALID_TYPE_EXTENSION

> Stability: 1 - Experimental

Attempted to execute a `.cjs` module with the `--type=module` flag.

<a id="ERR_INVALID_TYPE_FLAG"></a>
### ERR_INVALID_TYPE_FLAG

> Stability: 1 - Experimental

An invalid `--type=...` flag value was provided.

<a id="ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK"></a>
#### ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK

Expand Down Expand Up @@ -2238,7 +2252,6 @@ size.
This `Error` is thrown when a read is attempted on a TTY `WriteStream`,
such as `process.stdout.on('data')`.


[`'uncaughtException'`]: process.html#process_event_uncaughtexception
[`--force-fips`]: cli.html#cli_force_fips
[`Class: assert.AssertionError`]: assert.html#assert_class_assert_assertionerror
Expand Down
Loading