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

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

Closed
wants to merge 2 commits into from

Conversation

guybedford
Copy link
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

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>
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
Copy link
Collaborator

@nodejs-github-bot 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
Copy link
Contributor Author

This should probably go along with #26579.

@guybedford
Copy link
Contributor Author

//cc @ryzokuken

@bcoe
Copy link
Contributor

bcoe commented Apr 8, 2019

@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
Copy link
Contributor

bcoe commented Apr 16, 2019

@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).

BethGriggs pushed a commit that referenced this pull request Apr 30, 2019
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
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
Copy link
Member

Landed on v10.x-staging.

@BethGriggs BethGriggs closed this Apr 30, 2019
@ryzokuken
Copy link
Contributor

Thanks @guybedford and @BethGriggs

@bcoe
Copy link
Contributor

bcoe commented May 1, 2019

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

BethGriggs pushed a commit that referenced this pull request May 10, 2019
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
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
MylesBorins added a commit that referenced this pull request May 16, 2019
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
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
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
Refs: #27124
Refs: #25112

PR-URL: #27727
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants