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

src: expose MaybeInitializeContext to allow existing contexts #28544

Conversation

@MarshallOfSound
Copy link
Member

commented Jul 4, 2019

Splits the node.js specific tweak intialization of NewContext into a new helper MaybeInitializeContext so that embedders with existing contexts can still use them in a node Environment now that primordials are initialized and required so early.

Found this issue while upgrading Electron to node 12.4, now primordials are created in every context --> electron/node@3da36d0 we need to initialize a context the same way node does but we don't create the context so this method had to be split into two to allow folks with pre-created contexts to still use them in node Environments

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

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

src/node.h Outdated Show resolved Hide resolved
src: expose MaybeInitializeContext to allow existing contexts
Splits the node.js specific tweak intialization of NewContext into a new
helper MaybeInitializeContext so that embedders with existing contexts
can still use them in a node Environment now that primordials are
initialized and required so early.

@MarshallOfSound MarshallOfSound force-pushed the MarshallOfSound:expose-maybe-initialize-context branch from edddd5d to 98b6cf5 Jul 4, 2019

src/node.h Outdated
@@ -305,6 +305,12 @@ NODE_EXTERN v8::Local<v8::Context> NewContext(
v8::Local<v8::ObjectTemplate> object_template =
v8::Local<v8::ObjectTemplate>());

// Runs Node.js-specific tweaks on an already constructed context
NODE_EXTERN v8::Local<v8::Context> MaybeInitializeContext(

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jul 5, 2019

Member

For legacy reasons NewContext still need to return a Local, but for a new API I think we should return a MaybeLocal here. I would drop the word Maybe in the function name in favor of a more practical signature.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

Sorry @BridgeAR had PTO last week 🌴

I ended up with a slightly different approach here, I realized that MaybeInitializeContext was just returning either the context you passed in or a new empty maybe to indicate success / failure. It's more normal to use a boolean to indicate that so now InitializeContext returns a success boolean indicating whether or not the initialization succeeded.

@MarshallOfSound MarshallOfSound force-pushed the MarshallOfSound:expose-maybe-initialize-context branch from 9c6cacf to a954ce8 Jul 16, 2019

src/node.h Outdated Show resolved Hide resolved
src/api/environment.cc Outdated Show resolved Hide resolved

@MarshallOfSound MarshallOfSound force-pushed the MarshallOfSound:expose-maybe-initialize-context branch from a954ce8 to 2dcfc04 Jul 22, 2019

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

This can technically land but another review or two would be good I think. @addaleax @BridgeAR

I'm also going to re-run CI since it's been over a month...

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

Copy link

commented Aug 24, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Landed in 33ae95c

@Trott Trott closed this Aug 27, 2019

Trott added a commit that referenced this pull request Aug 27, 2019
src: expose MaybeInitializeContext to allow existing contexts
Splits the node.js specific tweak intialization of NewContext into a new
helper MaybeInitializeContext so that embedders with existing contexts
can still use them in a Node.js Environment now that primordials are
initialized and required so early.

Update MaybeInitializeContext to return MaybeLocal,

PR-URL: #28544
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 3, 2019
src: expose MaybeInitializeContext to allow existing contexts
Splits the node.js specific tweak intialization of NewContext into a new
helper MaybeInitializeContext so that embedders with existing contexts
can still use them in a Node.js Environment now that primordials are
initialized and required so early.

Update MaybeInitializeContext to return MaybeLocal,

PR-URL: #28544
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR referenced this pull request Sep 3, 2019
BridgeAR added a commit that referenced this pull request Sep 4, 2019
src: expose MaybeInitializeContext to allow existing contexts
Splits the node.js specific tweak intialization of NewContext into a new
helper MaybeInitializeContext so that embedders with existing contexts
can still use them in a Node.js Environment now that primordials are
initialized and required so early.

Update MaybeInitializeContext to return MaybeLocal,

PR-URL: #28544
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Sep 5, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 5, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 5, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 5, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: expose MaybeInitializeContext to allow existing contexts
Splits the node.js specific tweak intialization of NewContext into a new
helper MaybeInitializeContext so that embedders with existing contexts
can still use them in a Node.js Environment now that primordials are
initialized and required so early.

Update MaybeInitializeContext to return MaybeLocal,

PR-URL: nodejs#28544
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: expose MaybeInitializeContext to allow existing contexts
Splits the node.js specific tweak intialization of NewContext into a new
helper MaybeInitializeContext so that embedders with existing contexts
can still use them in a Node.js Environment now that primordials are
initialized and required so early.

Update MaybeInitializeContext to return MaybeLocal,

PR-URL: nodejs#28544
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.