Skip to content

src: expose granular SetIsolateUpForNode#30150

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.

Sign up for GitHub

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

Jump to bottom
Closed
codebytere wants to merge 1 commit into from
Closed

src: expose granular SetIsolateUpForNode #30150

codebytere wants to merge 1 commit into from

Conversation

@codebytere
Copy link
Member

@codebyterecodebytere commented Oct 27, 2019

This PR exposes a new embedder-focused API: SetIsolateUpForNode.

It maintains previous behavior for the single-param version of SetIsolateUpForNode and changes no defaults, but was designed to be flexible by allowing for embedders to conditionally override all callbacks and flags set by the previous two-param version of SetIsolateUpForNode.

cc @addaleax

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-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. labels Oct 27, 2019
devsnek
devsnek approved these changes Oct 27, 2019
View reviewed changes
devsnek
devsnek reviewed Oct 27, 2019
View reviewed changes
@Trott
Copy link
Member

Trott commented Oct 27, 2019

Since there's an existing API, I guess there's not a lot of flexibility on the name. But just in case I'm wrong about that: Might SetUpIsolateForNode be a better name than SetIsolateUpForNode? Or even ConfigureIsolateForNode?

@addaleaxaddaleax added embedding Issues and PRs related to embedding Node.js in another project. semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 28, 2019
addaleax
addaleax approved these changes Oct 28, 2019
View reviewed changes
Copy link
Member

@addaleaxaddaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 28, 2019

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

devsnek
devsnek reviewed Oct 29, 2019
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 31, 2019

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

@codebyterecodebytere added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 1, 2019
@codebytere
Copy link
MemberAuthor

codebytere commented Nov 1, 2019

Landed in fc02cf5

codebytere added a commit that referenced this pull request Nov 1, 2019
@codebytere
src: expose granular SetIsolateUpForNode
fc02cf5
This PR exposes a new embedder-focused API: SetIsolateUpForNode. It maintains previous behavior for the single-param version of SetIsolateUpForNode and changes no defaults, but was designed to be flexible by allowing for embedders to conditionally override all callbacks and flags set by the previous two-param version of SetIsolateUpForNode. PR-URL: #30150 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: David Carlier <[email protected]>
@codebyterecodebytere deleted the granular-setupisolate branch November 1, 2019 17:27
targos pushed a commit that referenced this pull request Nov 5, 2019
@codebytere@targos
src: expose granular SetIsolateUpForNode
a0df91c
This PR exposes a new embedder-focused API: SetIsolateUpForNode. It maintains previous behavior for the single-param version of SetIsolateUpForNode and changes no defaults, but was designed to be flexible by allowing for embedders to conditionally override all callbacks and flags set by the previous two-param version of SetIsolateUpForNode. PR-URL: #30150 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: David Carlier <[email protected]>
@targostargos mentioned this pull request Nov 5, 2019
Merged
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
@codebytere@MylesBorins
src: expose granular SetIsolateUpForNode
bf4c39d
This PR exposes a new embedder-focused API: SetIsolateUpForNode. It maintains previous behavior for the single-param version of SetIsolateUpForNode and changes no defaults, but was designed to be flexible by allowing for embedders to conditionally override all callbacks and flags set by the previous two-param version of SetIsolateUpForNode. PR-URL: #30150 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: David Carlier <[email protected]>
@targostargos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@codebytere@BethGriggs
src: expose granular SetIsolateUpForNode
455a643
This PR exposes a new embedder-focused API: SetIsolateUpForNode. It maintains previous behavior for the single-param version of SetIsolateUpForNode and changes no defaults, but was designed to be flexible by allowing for embedders to conditionally override all callbacks and flags set by the previous two-param version of SetIsolateUpForNode. PR-URL: #30150 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: David Carlier <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fhinkelfhinkelfhinkel approved these changes

@addaleaxaddaleaxaddaleax approved these changes

+2 more reviewers

@devsnekdevsnekdevsnek approved these changes

@devnexendevnexendevnexen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.embeddingIssues and PRs related to embedding Node.js in another project.lib / srcIssues and PRs related to general changes in the lib or src directory.semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@codebytere@Trott@nodejs-github-bot@fhinkel@addaleax@devnexen@devsnek