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.

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

src: expose granular SetIsolateUpForNode #30150

Closed
wants to merge 1 commit into from
Closed

src: expose granular SetIsolateUpForNode #30150

wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

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-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. labels Oct 27, 2019
devsnek
devsnek approved these changes Oct 27, 2019
View reviewed changes
src/api/environment.cc Show resolved Hide resolved
devsnek
devsnek reviewed Oct 27, 2019
View reviewed changes
src/node.h Show resolved Hide resolved
@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?

@addaleax addaleax 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

@addaleax addaleax 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

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

devsnek
devsnek reviewed Oct 29, 2019
View reviewed changes
src/node.h Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@codebytere codebytere 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
Member Author

Landed in fc02cf5

@codebytere codebytere closed this Nov 1, 2019
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]>
@codebytere codebytere 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]>
@targos targos 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]>
@targos targos 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]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@devnexen devnexen devnexen approved these changes

@addaleax addaleax addaleax approved these changes

@fhinkel fhinkel fhinkel approved these changes

Assignees
No one assigned
Labels
author ready PRs 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++. embedding Issues and PRs related to embedding Node.js in another project. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs 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