GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [v12 backport] lib: no need to strip BOM or shebang for scripts by aduh95 · Pull Request #31228 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 commented Jan 6, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. v12.x labels Jan 6, 2020
@devsnek @aduh95
lib: rework logic of stripping BOM+Shebang from commonjs
eb38365 
Fixes nodejs#27767

PR-URL: nodejs#27768
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@aduh95
aduh95 force-pushed the backport-27768-to-erbium branch from 978378f to 6667b45 Compare January 6, 2020 22:31
@refack @aduh95
lib: no need to strip BOM or shebang for scripts
7100e23 
PR-URL: nodejs#27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@aduh95
aduh95 force-pushed the backport-27768-to-erbium branch from 6667b45 to 7100e23 Compare January 6, 2020 23:27
@aduh95 aduh95 mentioned this pull request Jan 7, 2020
[v12 backport] vm: add Synthetic modules #31241
Closed
2 tasks done
@MylesBorins

MylesBorins commented Jan 7, 2020

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 7, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos closed this Jan 7, 2020
@targos targos reopened this Jan 7, 2020
@MylesBorins

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins mentioned this pull request Jan 8, 2020
lib: rework logic of stripping BOM+Shebang from commonjs #27768
Merged
3 tasks done
targos
targos approved these changes Jan 8, 2020
View reviewed changes
@targos

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Jan 8, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

@aduh95 thanks for the PR!
aduh95 reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos closed this Jan 8, 2020
targos pushed a commit that referenced this pull request Jan 8, 2020
@devsnek @targos
lib: rework logic of stripping BOM+Shebang from commonjs
3645b1b 
Fixes #27767

Backport-PR-URL: #31228
PR-URL: #27768
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Jan 8, 2020
@refack @targos
lib: no need to strip BOM or shebang for scripts
4c6bcda 
Backport-PR-URL: #31228
PR-URL: #27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jan 8, 2020
@devsnek @targos
lib: rework logic of stripping BOM+Shebang from commonjs
6c34ad6 
Fixes #27767

Backport-PR-URL: #31228
PR-URL: #27768
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Jan 8, 2020
@refack @targos
lib: no need to strip BOM or shebang for scripts
b743f2c 
Backport-PR-URL: #31228
PR-URL: #27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@devsnek @BethGriggs
lib: rework logic of stripping BOM+Shebang from commonjs
1c50714 
Fixes #27767

Backport-PR-URL: #31228
PR-URL: #27768
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@refack @BethGriggs
lib: no need to strip BOM or shebang for scripts
bcd27f7 
Backport-PR-URL: #31228
PR-URL: #27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@aduh95 @MylesBorins @nodejs-github-bot @targos @devsnek @refack
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.