FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v12 backport] lib: no need to strip BOM or shebang for scripts by aduh95 · Pull Request #31228 · nodejs/node · GitHub

/ node Public

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

Closed
aduh95 wants to merge 2 commits into
nodejs:v12.x-stagingfrom
aduh95:backport-27768-to-erbium
ConversationCommits (2)ChecksFiles changed
Closed

[v12 backport] lib: no need to strip BOM or shebang for scripts#31228
aduh95 wants to merge 2 commits into
nodejs:v12.x-stagingfrom
aduh95:backport-27768-to-erbium

Conversation

aduh95 commented Jan 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Jan 6, 2020
lib: rework logic of stripping BOM+Shebang from commonjs
eb38365 
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>
aduh95 force-pushed the backport-27768-to-erbium branch from 978378f to 6667b45 Compare January 6, 2020 22:31
lib: no need to strip BOM or shebang for scripts
7100e23 
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>
aduh95 force-pushed the backport-27768-to-erbium branch from 6667b45 to 7100e23 Compare January 6, 2020 23:27
aduh95 mentioned this pull request Jan 7, 2020
Closed
2 tasks

MylesBorins commented Jan 7, 2020

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot commented Jan 7, 2020

Copy link
Copy Markdown
Collaborator

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

targos commented Jan 7, 2020

Copy link
Copy Markdown
Member

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 closed this Jan 7, 2020
targos reopened this Jan 7, 2020

MylesBorins commented Jan 8, 2020

Copy link
Copy Markdown
Contributor

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

MylesBorins commented Jan 8, 2020

Copy link
Copy Markdown
Contributor

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

MylesBorins mentioned this pull request Jan 8, 2020
Merged
3 tasks

targos commented Jan 8, 2020

Copy link
Copy Markdown
Member

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 commented Jan 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

@aduh95 thanks for the PR!

targos closed this Jan 8, 2020
targos pushed a commit that referenced this pull request Jan 8, 2020
lib: rework logic of stripping BOM+Shebang from commonjs
3645b1b 
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
lib: no need to strip BOM or shebang for scripts
4c6bcda 
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
lib: rework logic of stripping BOM+Shebang from commonjs
6c34ad6 
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
lib: no need to strip BOM or shebang for scripts
b743f2c 
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
lib: rework logic of stripping BOM+Shebang from commonjs
1c50714 
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
lib: no need to strip BOM or shebang for scripts
bcd27f7 
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 deleted the backport-27768-to-erbium branch November 14, 2020 11:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL