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

process: allow StartExecution() to take a main script ID #25474

Closed
wants to merge 1 commit into from

Conversation

@joyeecheung
Copy link
Member

commented Jan 13, 2019

The idea is to allow the C++ layer to run arbitrary scripts
as the main script. This paves the way for

  • cctest of the execution of Node.js instances
  • Earlier handling of per-process CLI options that affect
    execution modes (those usually do not make sense for the
    embedders).
  • Targets like mkcodecache or mksnapshot.

Also moves the handling of _third_party_main.js into C++.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
process: allow StartExecution() to take a main script ID
The idea is to allow the C++ layer to run arbitrary scripts
as the main script. This paves the way for

- cctest of the execution of Node.js instances
- Earlier handling of per-process CLI options that affect
  execution modes (those usually do not make sense for the
  embedders).
- Targets like mkcodecache or mksnapshot.

Also moves the handling of `_third_party_main.js` into C++.

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

@nodejs-github-bot

This comment has been minimized.

Copy link

commented Jan 13, 2019

@joyeecheung sadly an error occured when I tried to trigger a build :(

Local<Value> main_script_v;
if (main_script_id == nullptr) {
// TODO(joyeecheung): make this mandatory - we may also create an overload
// for main_script that is a Local<Function>.

This comment has been minimized.

Copy link
@addaleax

addaleax Jan 13, 2019

Member

Just to make sure I understand this correctly, overload means an overload of StartExecution() that takes a Local<Function>? I like that idea. :)

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 13, 2019

Author Member

@addaleax Yeah, and maybe we could also use an overload with a C++ callback that takes some useful arguments (maybe Environment?)

// directory which will be executed instead of Node's normal loading.
if (NativeModule.exists('_third_party_main')) {
function startMainThreadExecution(mainScript) {
if (mainScript) {

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 13, 2019

Author Member

This bit here is temporary - I think it would be better to do the branching in C++ and directly compileAndCall the main scripts instead. For per-process CLI options like --help we could branch in node::Start() since they do not make a lot of sense for either embedders or the workers anyway. Although unfortunately for _third_party_main we have to keep a driver that wrap it into a process.nextTick() (or maybe there is a cleverer way out?)

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

pull bot pushed a commit to shakir-abdo/node that referenced this pull request Jan 15, 2019
process: allow StartExecution() to take a main script ID
The idea is to allow the C++ layer to run arbitrary scripts
as the main script. This paves the way for

- cctest of the execution of Node.js instances
- Earlier handling of per-process CLI options that affect
  execution modes (those usually do not make sense for the
  embedders).
- Targets like mkcodecache or mksnapshot.

Also moves the handling of `_third_party_main.js` into C++.

PR-URL: nodejs#25474
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

Landed in 2c7f4f4

BridgeAR added a commit that referenced this pull request Jan 16, 2019
process: allow StartExecution() to take a main script ID
The idea is to allow the C++ layer to run arbitrary scripts
as the main script. This paves the way for

- cctest of the execution of Node.js instances
- Earlier handling of per-process CLI options that affect
  execution modes (those usually do not make sense for the
  embedders).
- Targets like mkcodecache or mksnapshot.

Also moves the handling of `_third_party_main.js` into C++.

PR-URL: #25474
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
process: allow StartExecution() to take a main script ID
The idea is to allow the C++ layer to run arbitrary scripts
as the main script. This paves the way for

- cctest of the execution of Node.js instances
- Earlier handling of per-process CLI options that affect
  execution modes (those usually do not make sense for the
  embedders).
- Targets like mkcodecache or mksnapshot.

Also moves the handling of `_third_party_main.js` into C++.

PR-URL: nodejs#25474
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
process: allow StartExecution() to take a main script ID
The idea is to allow the C++ layer to run arbitrary scripts
as the main script. This paves the way for

- cctest of the execution of Node.js instances
- Earlier handling of per-process CLI options that affect
  execution modes (those usually do not make sense for the
  embedders).
- Targets like mkcodecache or mksnapshot.

Also moves the handling of `_third_party_main.js` into C++.

PR-URL: nodejs#25474
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.