-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v12 backport] lib: no need to strip BOM or shebang for scripts #31228
Conversation
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>
978378f
to
6667b45
Compare
PR-URL: nodejs#27375 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
6667b45
to
7100e23
Compare
/cc @targos should this land separately from the bump to 7.9? |
I'll have to look at it in detail. As I said in #27768, it was partially reverted in a change that landed in v12.x already. |
Wasn't the original change Semver-Major? can we land this without causing any issues? |
The changes LGTM and the new lines in @devsnek wdyt? |
Backport-PR-URL: #31228 PR-URL: #27375 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: #31228 PR-URL: #27375 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: #31228 PR-URL: #27375 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Node.js v12's V8 version has been bumped from 7.4 straight to 7.8 and missed 702331b (which was introduced with 7.5 upgrade).EDIT: I get this part wrong... As of V8 7.4, V8 implements the Hashbang Grammar proposal by default, so 702331b removed custom code that handled hashbang for scripts. As far as I can see, it has not been backported to v12 yet.I'm also backporting #27768 to ease the conflict resolution.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes