Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

shebang: #!/usr/bin/env node — a BOM issue #464

Closed
SMotaal opened this issue Jan 9, 2020 · 19 comments
Closed

shebang: #!/usr/bin/env node — a BOM issue #464

SMotaal opened this issue Jan 9, 2020 · 19 comments

Comments

@SMotaal
Copy link

SMotaal commented Jan 9, 2020

⁉️

Folks, quick question about shebang on macOS!

Package: ~/blah/blah/package.json

{
   // …
   "type": "module",
   // …
}

Module: ~/blah/blah/blah.js

#!/usr/bin/env node

console.log(process);

Command: node blah.js # in ~/blah/blah

(node:19422) ExperimentalWarning: The ESM module loader is experimental.
file:///~/blah/blah/blah.js:1
#!/usr/bin/env node
 ^
SyntaxError: Invalid or unexpected token
    at Loader.moduleStrategy 
       (internal/modules/esm/translators.js:83:18)
    at async link
       (internal/modules/esm/module_job.js:37:21)
When did that change?

Checks:

v13.x unflagged
v12.14.1 flagged
{ type: 'commonjs' } — how this was passing is beyond me 😊
blah.cjs

"I'm not getting that error in node.js 13.6.0… maybe an issue in darwin? I'm not able to reproduce this issue with 13.6.0 on linux x64, installed by nvm." — @coreyfarrell

without BOM (thanks @devsnek — it relates to a discrepancy when BOM is present).


Module: ~/blah/blah/blah.js

- #!/usr/bin/env node
+ // #!/usr/bin/env node

Command: node blah.js # in ~/blah/blah

process {
  version: 'v13.6.0',
  versions: {
    node: '13.6.0',
    v8: '7.9.317.25-node.26',
    uv: '1.34.0',
    zlib: '1.2.11',
    brotli: '1.0.7',
    ares: '1.15.0',
    modules: '79',
    nghttp2: '1.40.0',
    napi: '5',
    llhttp: '2.0.1',
    openssl: '1.1.1d',
    cldr: '36.0',
    icu: '65.1',
    tz: '2019c',
    unicode: '12.1'
  },
  arch: 'x64',
  platform: 'darwin',
  release: {
    name: 'node',
    sourceUrl: 'https://nodejs.org/download/release/v13.6.0/node-v13.6.0.tar.gz',
    headersUrl: 'https://nodejs.org/download/release/v13.6.0/node-v13.6.0-headers.tar.gz'
  },
  // …
  config: {
    target_defaults: {
      cflags: [],
      default_configuration: 'Release',
      defines: [],
      include_dirs: [],
      libraries: []
    },
    // …
  },
  // …
  env: {
    // no NODE_… stuff
  },
  // …
}
@jkrems
Copy link
Contributor

jkrems commented Jan 9, 2020

I assume the same thing happens if the file is called blah.mjs (removing the specifics of the package.json field from the repro)?

@jkrems
Copy link
Contributor

jkrems commented Jan 9, 2020

One quick meta note is that V8 should support the shebang natively and IIRC our module implementation depends on that. So it feels really weird that it would be platform-dependent..?

@jkrems
Copy link
Contributor

jkrems commented Jan 9, 2020

Tried on my mac (version installed via nvm, so official release build) with both .js and .mjs. In both cases it seemed to work. Are there any other details I might have missed?

$ node test.js
(node:12797) ExperimentalWarning: The ESM module loader is experimental.
file:///tmp/shebang/test.js darwin
$ node -v
v13.6.0
$ cat test.js
#!/usr/bin/env node

import {platform} from 'os';

console.log(import.meta.url, platform());

@jkrems
Copy link
Contributor

jkrems commented Jan 9, 2020

Ah, missed { type: 'commonjs' } (!). Wait - if type is CommonJS, why is it trying to run the source as a module?

P.S.: internal/modules/esm/translators.js:83 on v13.6.0 is the handler for files in module format which shouldn't apply to CJS source code.

@jkrems
Copy link
Contributor

jkrems commented Jan 9, 2020

Still, same result after running as CJS (removing my bad type field from package.json):

$ cat test.js
#!/usr/bin/env node

const {platform} = require('os');

console.log(platform());
$ node test.js
darwin
$ node -v
v13.6.0

@GeoffreyBooth
Copy link
Member

I haven't tried debugging this, but I remember that earlier versions of our loader included Node stripping out the shebang line; I think when V8 added support for it, Node's code was removed. So there might be a bug in V8 shebang handling.

@devsnek
Copy link
Member

devsnek commented Jan 9, 2020

@SMotaal does your file start with a utf8 byte order mark?

echo \uFEFF#!test | v8
V8 version 8.1.117
d8> (d8):1: SyntaxError: Invalid or unexpected token
#!test
 ^
SyntaxError: Invalid or unexpected token

@SMotaal
Copy link
Author

SMotaal commented Jan 9, 2020

Yup, that does it :)

I default to this in vscode:

Screen Shot 2020-01-09 at 2 23 34 PM

I saved as UTF-8 (no BOM) and it worked!

@ljharb
Copy link
Member

ljharb commented Jan 9, 2020

Is it intended that a starting BOM would not be removed as part of the shebang?

@devsnek
Copy link
Member

devsnek commented Jan 9, 2020

it shouldn't be removed as part of the hashbang, but i'm surprised that node doesn't strip utf8 bom when reading source files anymore.

@SMotaal
Copy link
Author

SMotaal commented Jan 9, 2020

I am inclined to say it is an OS problem:

bom.sh

#!/usr/bin/env sh

echo "executed!";
  • with BOM:

    ./bom.sh: line 1: #!/usr/bin/env: No such file or directory
    executed!
    
  • without BOM:

    executed!
    

@SMotaal
Copy link
Author

SMotaal commented Jan 9, 2020

which is more disturbing honestly :)

@devsnek
Copy link
Member

devsnek commented Jan 9, 2020

This issue isn't inherent to macos (and it technically isn't even an issue).

(on my linux machine)

What happened is that node.js used to strip utf8 bom before passing the source to vm, and now it doesn't. I don't think this is inherently bad, but I'm surprised we changed it.

@SMotaal
Copy link
Author

SMotaal commented Jan 9, 2020

I guess I used UTF-16 at some point and decided it was still to much hassle and went to UTF-8 which technically should not need BOM — ah living in the 🙃

But that issue comes up a lot apparently elsewhere too.

Okay, while we are here let's ask, folks, what is the recommended encode setting for ESM files?

@SMotaal SMotaal changed the title esm: #!/usr/bin/env node (macOS) esm: #!/usr/bin/env node — a BOM issue Jan 9, 2020
@SMotaal SMotaal closed this as completed Jan 9, 2020
@SMotaal SMotaal added cjs and removed bug? labels Jan 9, 2020
@SMotaal SMotaal changed the title esm: #!/usr/bin/env node — a BOM issue shebang: #!/usr/bin/env node — a BOM issue Jan 9, 2020
@SMotaal
Copy link
Author

SMotaal commented Jan 9, 2020

Thanks folks!

@ljharb
Copy link
Member

ljharb commented Jan 9, 2020

I assume everything should be utf-8 everywhere always, but it still seems like a BOM should be stripped.

@SMotaal
Copy link
Author

SMotaal commented Jan 9, 2020

@ljharb I think the problem is the OS is not stripping it, hence never called node — https://en.wikipedia.org/wiki/Shebang_(Unix)#Magic_number

+1 on UTF-8 — just can't recall why I was inclined to add BOMs exactly (assuming it was latent from UTF-16 bugging in Windows, but since moved to UTF-8)

@mbwhite
Copy link

mbwhite commented Apr 2, 2020

This issues related to the issue seen with rewire (jhnns/rewire#179)
It started occurring on node 12.16.0.

Would somebody be able to clarify how this issue relates please?

@guybedford
Copy link
Contributor

@mbwhite seems like that is due to this refactoring PR being backported - https://github.com/nodejs/node/pull/27768/files.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants