★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/25320Nouvelle recherche | Portail wanayoo
Skip to content

/node

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: split `LoadEnvironment()` at `startExecution()` #25320

Closed
wants to merge 2 commits into from
+47 −32

Conversation

@addaleax
Copy link
Member

commented Jan 2, 2019

This makes it easier to cater to embedders which wish to skip
the startExecution() part.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@addaleax
src: split `LoadEnvironment()` at `startExecution()` 
This makes it easier to cater to embedders which wish to skip
the `startExecution()` part.
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
1351c49

@addaleax addaleax requested a review from joyeecheung Jan 2, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jan 2, 2019

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2129/pipeline

@nodejs-github-bot nodejs-github-bot added C++ lib / src labels Jan 2, 2019

@joyeecheung
joyeecheung approved these changes Jan 3, 2019
View changes
Copy link
Member

left a comment

LGTM. I wonder if as a follow up we should put the JS land startExecution into a separate file..
@bnoordhuis
bnoordhuis approved these changes Jan 3, 2019
View changes
src/node.cc Outdated Show resolved Hide resolved
src/node.cc
if (start_execution.IsEmpty()) return;
start_execution->Call(
env->context(), Undefined(env->isolate()), 0, nullptr);
env->set_start_execution_function(Local<Function>());

This comment has been minimized.

Sign in to view
Copy link
@bnoordhuis

bnoordhuis Jan 3, 2019

Member

Maybe do this before the call to, er, Call() as an extra guard against recursive invocation?

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Jan 3, 2019

Author Member

Done, but it requires some extra magic because of the way the env->...() getters work…
@JungMinu
JungMinu approved these changes Jan 3, 2019
View changes
@cjihrig
cjihrig approved these changes Jan 3, 2019
View changes
@jasnell
jasnell approved these changes Jan 3, 2019
View changes
@addaleax
fixup! src: split `LoadEnvironment()` at `startExecution()`
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
eb913cd
@fhinkel
fhinkel approved these changes Jan 5, 2019
View changes
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 5, 2019

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

@addaleax addaleax added the author ready label Jan 5, 2019

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 6, 2019

Landed in f6a1d88

@addaleax addaleax closed this Jan 6, 2019

@addaleax addaleax deleted the addaleax:embedder-api branch Jan 6, 2019

addaleax added a commit that referenced this pull request Jan 6, 2019
@addaleax
src: split `LoadEnvironment()` at `startExecution()` 
This makes it easier to cater to embedders which wish to skip
the `startExecution()` part.

PR-URL: #25320
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
f6a1d88
@kkty kkty referenced this pull request Jan 8, 2019
Closed
2 of 2 tasks complete

@addaleax addaleax added the backport-requested-v11.x label Jan 9, 2019

@BridgeAR BridgeAR added this to Backport requested in v11.x Jan 10, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019
Anna Henningsen @refack
src: split `LoadEnvironment()` at `startExecution()` 
This makes it easier to cater to embedders which wish to skip
the `startExecution()` part.

PR-URL: nodejs#25320
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
26c626b

@addaleax addaleax removed the backport-requested-v11.x label Jan 15, 2019

addaleax added a commit that referenced this pull request Jan 15, 2019
@addaleax
src: split `LoadEnvironment()` at `startExecution()` 
This makes it easier to cater to embedders which wish to skip
the `startExecution()` part.

PR-URL: #25320
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
21096f9
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
Merged
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@addaleax @BridgeAR
src: split `LoadEnvironment()` at `startExecution()` 
This makes it easier to cater to embedders which wish to skip
the `startExecution()` part.

PR-URL: nodejs#25320
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
e0d2fb3
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
@addaleax @BridgeAR
src: split `LoadEnvironment()` at `startExecution()` 
This makes it easier to cater to embedders which wish to skip
the `startExecution()` part.

PR-URL: nodejs#25320
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
6cdaf03
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Merged

@targos targos removed this from Backport requested in v11.x Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fhinkel fhinkel

@bnoordhuis bnoordhuis

@jasnell jasnell

@cjihrig cjihrig

@joyeecheung joyeecheung

@JungMinu JungMinu

Assignees
No one assigned
Labels
C++ author ready lib / src
Projects
None yet
Milestone
No milestone
8 participants
@addaleax @nodejs-github-bot @fhinkel @bnoordhuis @jasnell @cjihrig @joyeecheung @JungMinu
You can’t perform that action at this time.