Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 6, 2020

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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Jan 6, 2020
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>
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>
@MylesBorins
Copy link
Member

/cc @targos should this land separately from the bump to 7.9?

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Jan 7, 2020

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.
We have to make sure this doesn't cancel more recent changes.

@targos targos closed this Jan 7, 2020
@targos targos reopened this Jan 7, 2020
@MylesBorins
Copy link
Member

Wasn't the original change Semver-Major? can we land this without causing any issues?

@MylesBorins
Copy link
Member

also worth noting @targos the backport of 7.5 didn't include the partial revert #28005

@targos
Copy link
Member

targos commented Jan 8, 2020

The changes LGTM and the new lines in test/sequential/test-module-loading.js already pass with current v12.x so it doesn't look like this is breaking.

@devsnek wdyt?

@targos
Copy link
Member

targos commented Jan 8, 2020

Landed in 3645b1b and 4c6bcda. Let me know if I shouldn't have.

@aduh95 thanks for the PR!

@targos targos closed this Jan 8, 2020
targos pushed a commit that referenced this pull request Jan 8, 2020
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>
targos pushed a commit that referenced this pull request Jan 8, 2020
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>
targos pushed a commit that referenced this pull request Jan 8, 2020
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>
targos pushed a commit that referenced this pull request Jan 8, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
@aduh95 aduh95 deleted the backport-27768-to-erbium branch November 14, 2020 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants