Skip to content

Break out platform delegate - #30324

Closed
laverdet wants to merge 1 commit into
nodejs:masterfrom
laverdet:platform-delegate
Closed

Break out platform delegate#30324
laverdet wants to merge 1 commit into
nodejs:masterfrom
laverdet:platform-delegate

Conversation

@laverdet

@laverdetlaverdet commented Nov 7, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

This is continuation from the discussion here: laverdet/isolated-vm#133.

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

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 7, 2019
@laverdet

laverdet commented Nov 7, 2019

Copy link
Copy Markdown
ContributorAuthor

For posterity previous inline comments and responses are here: laverdet@8623e9a

addaleax
addaleax approved these changes Nov 7, 2019
View reviewed changes
Comment threadsrc/node_platform.cc Outdated
addaleax
addaleax reviewed Nov 7, 2019
View reviewed changes
Comment threadsrc/node.h Outdated
@joyeecheung

joyeecheung commented Nov 7, 2019

Copy link
Copy Markdown
Member

Can you add a cctest for this? (I think using a PerIsolatePlatformData as the delegate should already work?)

lundibundi
lundibundi reviewed Nov 7, 2019
View reviewed changes
Comment threadsrc/node.h Outdated
bnoordhuis
bnoordhuis reviewed Nov 7, 2019
View reviewed changes
Comment threadsrc/node_platform.cc Outdated
bnoordhuis
bnoordhuis approved these changes Nov 12, 2019
View reviewed changes
Comment threadsrc/node.h Outdated
Comment threadsrc/node_platform.cc Outdated
@laverdet
embedding: add abstract IsolatePlatformDelegate
5164b20
Adds a new abstract class for module authors and embedders to register
arbitrary isolates with `node::MultiIsolatePlatform`.
@addaleaxaddaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 13, 2019
@addaleax

addaleax commented Nov 13, 2019

Copy link
Copy Markdown
Member

Now that this has definitely become semver-major this needs another @nodejs/tsc review…

@nodejs-github-bot

nodejs-github-bot commented Nov 13, 2019

Copy link
Copy Markdown
Collaborator

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

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 18, 2019
@nodejs-github-bot

nodejs-github-bot commented Nov 18, 2019

Copy link
Copy Markdown
Collaborator

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

addaleax pushed a commit that referenced this pull request Nov 20, 2019
@laverdet@addaleax
src: add abstract IsolatePlatformDelegate
c712fb7
Adds a new abstract class for module authors and embedders to register
arbitrary isolates with `node::MultiIsolatePlatform`.
PR-URL: #30324
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@addaleax

addaleax commented Nov 20, 2019

Copy link
Copy Markdown
Member

Landed in c712fb7, thanks for the pull request and please let us know if there’s anything else you need! 🚀

@addaleaxaddaleax mentioned this pull request Nov 28, 2019
Closed
@bnoordhuisbnoordhuis mentioned this pull request Dec 8, 2019
Closed
@addaleaxaddaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020
@BethGriggsBethGriggs mentioned this pull request Mar 10, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes
@bnoordhuisbnoordhuisbnoordhuis approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
+1 more reviewer
@lundibundilundibundilundibundi left review comments
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++.semver-majorPRs that contain breaking changes and should be released in the next major version.v8 platformIssues and PRs related to Node's v8::Platform implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@laverdet@joyeecheung@addaleax@nodejs-github-bot@bnoordhuis@lundibundi