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

[v10.x-backport] module: use compileFunction over Module.wrap by guybedford · Pull Request #27124 · nodejs/node · GitHub

/ node Public

[v10.x-backport] module: use compileFunction over Module.wrap - #27124

Closed
guybedford wants to merge 2 commits into
nodejs:v10.x-stagingfrom
guybedford:backport-21573
ConversationCommits (2)ChecksFiles changed
Closed

[v10.x-backport] module: use compileFunction over Module.wrap#27124
guybedford wants to merge 2 commits into
nodejs:v10.x-stagingfrom
guybedford:backport-21573

Conversation

guybedford commented Apr 7, 2019

Copy link
Copy Markdown
Contributor

This backports PR #21573, which replaces the CJS wrapper with the use of compileFunction.

The major benefit of this is that it will allow us to use native v8 coverage support in the 10.x branch.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

ryzokuken added 2 commits April 7, 2019 20:21
module: use compileFunction over Module.wrap
4e6f8ab 
Use vm.compileFunction (which is a binding for
v8::CompileFunctionInContext) instead of Module.wrap internally in
Module._compile for the cjs loader.

Fixes: nodejs#17396

PR-URL: nodejs#21573
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
module: revert module._compile to original state if module is patched
52289fb 
PR-URL: nodejs#21573
Fixes: nodejs#17396
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot commented Apr 7, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v10.x labels Apr 7, 2019

guybedford commented Apr 7, 2019

Copy link
Copy Markdown
Contributor Author

This should probably go along with #26579.

guybedford commented Apr 8, 2019

Copy link
Copy Markdown
Contributor Author

//cc @ryzokuken

bcoe commented Apr 8, 2019

Copy link
Copy Markdown
Contributor

@guybedford 👋 thanks for putting this work in, this will help make coverage a much more consistent experience once Node 8 cycles out; I'll try to do some end to end testing for you this week.

bcoe commented Apr 16, 2019

Copy link
Copy Markdown
Contributor

@nodejs/releasers 👋 it would be awesome to get this back-ported, as it makes the behavior between Node 10 and newer Node consistent, in relation to byte offsets (really valuable for coverage).

bcoe mentioned this pull request Apr 18, 2019
Closed
BethGriggs pushed a commit that referenced this pull request Apr 30, 2019
module: use compileFunction over Module.wrap
8f84ae0 
Use vm.compileFunction (which is a binding for
v8::CompileFunctionInContext) instead of Module.wrap internally in
Module._compile for the cjs loader.

Fixes: #17396

Backport-PR-URL: #27124
PR-URL: #21573
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request Apr 30, 2019
module: revert module._compile to original state if module is patched
9101d93 
Backport-PR-URL: #27124
PR-URL: #21573
Fixes: #17396
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

BethGriggs commented Apr 30, 2019

Copy link
Copy Markdown
Member

Landed on v10.x-staging.

BethGriggs closed this Apr 30, 2019

ryzokuken commented Apr 30, 2019

Copy link
Copy Markdown
Contributor

Thanks @guybedford and @BethGriggs

bcoe commented May 1, 2019

Copy link
Copy Markdown
Contributor

@BethGriggs @guybedford I am very excited about this \o/

This was referenced May 1, 2019
Merged
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
module: use compileFunction over Module.wrap
1a15fdc 
Use vm.compileFunction (which is a binding for
v8::CompileFunctionInContext) instead of Module.wrap internally in
Module._compile for the cjs loader.

Fixes: #17396

Backport-PR-URL: #27124
PR-URL: #21573
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request May 10, 2019
module: revert module._compile to original state if module is patched
dcf9519 
Backport-PR-URL: #27124
PR-URL: #21573
Fixes: #17396
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins added a commit to MylesBorins/node that referenced this pull request May 16, 2019
test: fix expectation in test-bootstrap-modules
4bfb258 
Refs: nodejs#27124
Refs: nodejs#25112
MylesBorins mentioned this pull request May 16, 2019
Closed
MylesBorins added a commit that referenced this pull request May 16, 2019
test: fix expectation in test-bootstrap-modules
d19c2c5 
Refs: #27124
Refs: #25112

PR-URL: #27727
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
module: use compileFunction over Module.wrap
590e8d3 
Use vm.compileFunction (which is a binding for
v8::CompileFunctionInContext) instead of Module.wrap internally in
Module._compile for the cjs loader.

Fixes: #17396

Backport-PR-URL: #27124
PR-URL: #21573
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
module: revert module._compile to original state if module is patched
a49bd36 
Backport-PR-URL: #27124
PR-URL: #21573
Fixes: #17396
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins added a commit that referenced this pull request May 16, 2019
test: fix expectation in test-bootstrap-modules
7b76acb 
Refs: #27124
Refs: #25112

PR-URL: #27727
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced Jan 12, 2022
Open
Open
This was referenced Jan 12, 2022
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Feb 9, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
kissofire mentioned this pull request May 22, 2024
Open
137717unity mentioned this pull request May 22, 2024
Open
wedataintelligence mentioned this pull request May 22, 2024
Open
baby636 mentioned this pull request May 22, 2024
Open
aliceUnhinged613 mentioned this pull request May 23, 2024
Open
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

BethGriggs BethGriggs approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. 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.

5 participants

Back | FazBrowse Home | New Git URL