lib: rework logic of stripping BOM+Shebang from commonjs #27768

devsnek · 2019-05-19T04:10:45Z

Fixes #27767

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2019-05-19T04:10:47Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3562

mscdex · 2019-05-19T04:36:05Z

Test?

guybedford · 2019-05-19T05:08:24Z

Does this mean BOM + shebang will now fail? Isn't that a breaking change?

silverwind · 2019-05-19T07:55:05Z

Imo it should strip both, possibly even more than once (e.g. when a file for some reason has 2 BOMs or 2 shebangs).

devsnek · 2019-05-19T13:11:21Z

@silverwind a bom/shebang that doesn't occur as the first bytes of a file isn't a bom/shebang. it's just a collection of bytes that happens to look like a bom/shebang.

silverwind · 2019-05-19T13:40:53Z

I remember having trouble running a script once with two shebangs, like

#!/usr/bin/env node
#!/usr/bin/env node

You could probably write it off as a unsupported edge case.

Double BOM, I have no example, but I could see it happen from sloppily written software.

lib/internal/modules/cjs/helpers.js

BridgeAR · 2019-05-19T20:19:58Z

A test case would be good and there's an ongoing discussion in tc39/proposal-hashbang#14. It seems like we are relatively flexible in this case since Node.js could handle these similar to how browsers do but we could also decide to be strict about them as suggested here.

BridgeAR · 2019-05-19T20:21:47Z

@silverwind having multiple shebangs always seems to be a bug and I don't think we should handle this gracefully.

Trott · 2019-05-20T06:34:49Z

/ping @bakkot

lib/internal/modules/cjs/helpers.js

bakkot · 2019-05-20T06:53:59Z

Approach seems fine to me. It is a breaking change, technically, as noted above.

Putting the logic to strip a BOM in _compile means it will get run for other consumers of that method, but what appears to be the only other consumer is prepending stuff anyway, which makes it irrelevant.

devsnek · 2019-05-20T16:29:20Z

fun sidenote: github doesn't recognize a shebang after a bom as a shebang:

nodejs-github-bot · 2019-05-20T21:27:45Z

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

BridgeAR

LGTM while I would also be fine to accept the BOM before a shebang (for similar reasons as discussed in the TC39 issue).

Trott · 2019-05-20T23:50:16Z

CITGM on a few platforms (figure this doesn't need to be run on everything under the sun?): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1853/

Trott

LGTM if CITGM doesn't reveal actual dependence on this behavior...

joyeecheung · 2019-05-21T15:36:26Z

I don't think the issue comes from how we strip the BOM - with hashbang shipping in V8, whether the hashbang is ignored is up to the JS parser.

The behavior difference comes from the fact that in -c we still manually wrap the source code and then use new vm.Script to compile the script, so we are the one who strips it. For V8 it always sees the wrapper to be at the beginning of the source. Whereas when the file is loaded as a normal module, we use CompileFunctionInContext and pass the arguments from the API instead of through a wrapper. Even if we do not strip it ourselves, and just strip the BOM before passing it to V8, V8 is now going to ignore the shebang for us.

I think the correct fix should be to use CompileFunctionInContext in check_syntax.js instead, and just strip the BOM before passing the source to V8.

devsnek · 2019-05-21T15:43:50Z

@joyeecheung i agree at a high level, this pr is more about making our handling of stripping consistent between run and check. i'd like to possibly revisit bom stripping in general, as it seems more like something that would happen at file read time than at module compilation time, and we'll need to look at the behaviour of shebang stripping again when v8 enables it by default.

BridgeAR · 2019-06-17T17:16:55Z

@devsnek looking at the code it seems to me this could be partially backported to v12.x. Would you be so kind and open a backport for this where you cherry-pick this commit and add a compatibility patch on top that removes the actual breaking change?

devsnek · 2019-06-17T17:25:19Z

@BridgeAR like backport the stripShebangOrBOM function but don't actually use it?

BridgeAR · 2019-06-17T17:38:45Z

@devsnek yes. It's pretty much about keeping as many changed code lines as they are while removing the actual breaking change.

targos · 2019-06-17T17:42:59Z

Note that the V8 7.5 update (#27375), which has already been backported to 12.x-staging partially reverted this PR

Notable changes: - **assert**: - do not repeat .throws() code (Ruben Bridgewater) [#28263](#28263) - wrap validation function errors (Ruben Bridgewater) [#28263](#28263) - fix generatedMessage property (Ruben Bridgewater) [#28263](#28263) - improve class instance errors (Ruben Bridgewater) [#28263](#28263) - **benchmark**: - use test/common/tmpdir consistently (João Reis) [#28858](#28858) - **build**: - make full-icu the default for releases (Richard Lau) [#29887](#29887) - update minimum Xcode version for macOS (Michael Dawson) [#29622](#29622) - **child_process**: - runtime deprecate \_channel (cjihrig) [#27949](#27949) - simplify spawn argument parsing (cjihrig) [#27854](#27854) - **console**: - display timeEnd with suitable time unit (Xavier Stouder) [#29251](#29251) - **deps**: - patch V8 to 7.8.279.14 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.12 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.10 (Myles Borins) [#29694](#29694) - update V8's postmortem script (cjihrig) [#29694](#29694) - V8: cherry-pick 716875d (Myles Borins) [#29694](#29694) - update V8 to 7.8.279.9 (Myles Borins) [#29694](#29694) - V8: cherry-pick b33af60 (Michaël Zasso) [#28016](#28016) - update V8 to 7.6.303.28 (Michaël Zasso) [#28016](#28016) - **domain**: - error handler runs outside of its domain (Julien Gilli) [#26211](#26211) - **fs**: - make FSWatcher.start private (Lucas Holmquist) [#29905](#29905) - add runtime deprecate for file stream open() (Robert Nagy) [#29061](#29061) - allow int64 offset in fs.write/writeSync/fd.write (Zach Bjornson) [#26572](#26572) - use IsSafeJsInt instead of IsNumber for ftruncate (Zach Bjornson) [#26572](#26572) - allow int64 offset in fs.read/readSync/fd.read (Zach Bjornson) [#26572](#26572) - close file descriptor of promisified truncate (João Reis) [#28858](#28858) - **http**: - do not emit end after aborted (Robert Nagy) [#27984](#27984) - don't emit 'data' after 'error' (Robert Nagy) [#28711](#28711) - remove legacy parser (Anna Henningsen) [#29589](#29589) - throw if 'host' agent header is not a string value (Giorgos Ntemiris) [#29568](#29568) - replace superfluous connection property with getter/setter (Robert Nagy) [#29015](#29015) - fix test where aborted should not be emitted (Robert Nagy) [#20077](#20077) - remove default 'timeout' listener on upgrade (Luigi Pinca) [#26030](#26030) - **http, http2**: - remove default server timeout (Ali Ijaz Sheikh) [#27558](#27558) - **http2**: - remove security revert flags (Anna Henningsen) [#29141](#29141) - remove callback-based padding (Anna Henningsen) [#29144](#29144) - **lib**: - rename validateInteger to validateSafeInteger (Zach Bjornson) [#26572](#26572) - correct error.errno to always be numeric (Joyee Cheung) [#28140](#28140) - no need to strip BOM or shebang for scripts (Refael Ackermann) [#27375](#27375) - rework logic of stripping BOM+Shebang from commonjs (Gus Caplan) [#27768](#27768) - **module**: - runtime deprecate createRequireFromPath() (cjihrig) [#27951](#27951) - **readline**: - error on falsy values for callback (Sam Roberts) [#28109](#28109) - **repl**: - close file descriptor of history file (João Reis) [#28858](#28858) - **src**: - bring 425 status code name into accordance with RFC 8470 (Sergei Osipov) [#29880](#29880) - update NODE\_MODULE\_VERSION to 79 (Myles Borins) [#29694](#29694) - update NODE\_MODULE\_VERSION to 78 (Michaël Zasso) [#28918](#28918) - add error codes to errors thrown in C++ (Yaniv Friedensohn) [#27700](#27700) - use non-deprecated overload of V8::SetFlagsFromString (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 77 (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 74 (Refael Ackermann) [#27375](#27375) - make process.env.TZ setter clear tz cache (Ben Noordhuis) [#20026](#20026) - enable V8's WASM trap handlers (Gus Caplan) [#27246](#27246) - **stream**: - throw unhandled error for readable with autoDestroy (Robert Nagy) [#29806](#29806) - always invoke callback before emitting error (Robert Nagy) [#29293](#29293) - invoke callback before emitting error always (Robert Nagy) [#29293](#29293) - do not flush destroyed writable (Robert Nagy) [#29028](#29028) - don't emit finish on error (Robert Nagy) [#28979](#28979) - disallow stream methods on finished stream (Robert Nagy) [#28687](#28687) - do not emit after 'error' (Robert Nagy) [#28708](#28708) - fix destroy() behavior (Robert Nagy) [#29058](#29058) - simplify `.pipe()` and `.unpipe()` in Readable (Weijia Wang) [#28583](#28583) - **tools**: - patch V8 to run on older XCode versions (Ujjwal Sharma) [#29694](#29694) - update V8 gypfiles (Michaël Zasso) [#29694](#29694) - support full-icu by default (Steven R. Loomis) [#29522](#29522) - **util**: validate formatWithOptions inspectOptions (Ruben Bridgewater) [#29824](#29824) PR-URL: #29504

Fixes nodejs#27767 PR-URL: nodejs#27768 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins · 2020-01-08T01:51:31Z

the 7.5 backport PR didn't actually include either this change or the partial revert

#31228 has been opened to backport but I would like to confirm that it is not breaking before landing it

Fixes #27767 Backport-PR-URL: #31228 PR-URL: #27768 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

BridgeAR reviewed May 19, 2019

View reviewed changes

lib/internal/modules/cjs/helpers.js Outdated Show resolved Hide resolved

bakkot reviewed May 20, 2019

View reviewed changes

lib/internal/modules/cjs/helpers.js Outdated Show resolved Hide resolved

devsnek force-pushed the fix/bom-shebang-things branch from 6dfee0b to 18e68a8 Compare May 20, 2019 16:15

devsnek added module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels May 20, 2019

devsnek force-pushed the fix/bom-shebang-things branch from 18e68a8 to 1918d38 Compare May 20, 2019 16:28

devsnek requested a review from BridgeAR May 20, 2019 21:26

BridgeAR approved these changes May 20, 2019

View reviewed changes

devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 20, 2019

devsnek requested a review from a team May 20, 2019 22:08

Trott approved these changes May 20, 2019

View reviewed changes

devsnek force-pushed the fix/bom-shebang-things branch 4 times, most recently from 11807f4 to 0f42dc8 Compare May 21, 2019 20:56

devsnek added dont-land-on-v11.x labels May 26, 2019

BridgeAR added backport-requested-v12.x and removed dont-land-on-v12.x labels Jun 17, 2019

BridgeAR added backported-to-v12.x and removed backport-requested-v12.x labels Aug 6, 2019

BethGriggs mentioned this pull request Sep 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

aduh95 mentioned this pull request Jan 6, 2020

[v12 backport] lib: no need to strip BOM or shebang for scripts #31228

Closed

2 tasks

MylesBorins added backport-requested-v12.x and removed backported-to-v12.x labels Jan 8, 2020

targos added backported-to-v12.x and removed backport-requested-v12.x labels Jan 8, 2020

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

targos mentioned this pull request Jun 22, 2021

Run script from the console with UTF-8 with BOM #39111

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: rework logic of stripping BOM+Shebang from commonjs #27768

lib: rework logic of stripping BOM+Shebang from commonjs #27768

devsnek commented May 19, 2019 •

edited

Loading

nodejs-github-bot commented May 19, 2019

mscdex commented May 19, 2019

guybedford commented May 19, 2019

silverwind commented May 19, 2019 •

edited

Loading

devsnek commented May 19, 2019

silverwind commented May 19, 2019 •

edited

Loading

BridgeAR commented May 19, 2019

BridgeAR commented May 19, 2019

Trott commented May 20, 2019

bakkot commented May 20, 2019

devsnek commented May 20, 2019

nodejs-github-bot commented May 20, 2019

BridgeAR left a comment

Trott commented May 20, 2019

Trott left a comment

joyeecheung commented May 21, 2019 •

edited

Loading

devsnek commented May 21, 2019

BridgeAR commented Jun 17, 2019

devsnek commented Jun 17, 2019

BridgeAR commented Jun 17, 2019

targos commented Jun 17, 2019

MylesBorins commented Jan 8, 2020

lib: rework logic of stripping BOM+Shebang from commonjs #27768

lib: rework logic of stripping BOM+Shebang from commonjs #27768

Conversation

devsnek commented May 19, 2019 • edited Loading

Checklist

nodejs-github-bot commented May 19, 2019

mscdex commented May 19, 2019

guybedford commented May 19, 2019

silverwind commented May 19, 2019 • edited Loading

devsnek commented May 19, 2019

silverwind commented May 19, 2019 • edited Loading

BridgeAR commented May 19, 2019

BridgeAR commented May 19, 2019

Trott commented May 20, 2019

bakkot commented May 20, 2019

devsnek commented May 20, 2019

nodejs-github-bot commented May 20, 2019

BridgeAR left a comment

Choose a reason for hiding this comment

Trott commented May 20, 2019

Trott left a comment

Choose a reason for hiding this comment

joyeecheung commented May 21, 2019 • edited Loading

devsnek commented May 21, 2019

BridgeAR commented Jun 17, 2019

devsnek commented Jun 17, 2019

BridgeAR commented Jun 17, 2019

targos commented Jun 17, 2019

MylesBorins commented Jan 8, 2020

devsnek commented May 19, 2019 •

edited

Loading

silverwind commented May 19, 2019 •

edited

Loading

silverwind commented May 19, 2019 •

edited

Loading

joyeecheung commented May 21, 2019 •

edited

Loading