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: allow embedders to disable esm loader #34060

Closed

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jun 26, 2020

This PR addresses a recent embedder-specific issue Electron is facing.

Blink has its own independent ESM loader, which is set up in the renderer process of Electron. When ESM was unilaterally unflagged, this meant that there would be two competing ESM loaders in the same process, both of which made assumptions about code being run.

For example, what can happen is that the devtools windows uses Node.js' implementation of the esmodule loader, which is at present incapable of loading modules from URLs (for good reason). This causes a bunch of devtools failures. There's no real way to specifically enable sandbox (and therefore force the Blink loader to take precedence) for devtools only, since at the time we're calling CreateContentRendererClient() we don't know what url we're loading (and therefore can't verify it's devtools or not). Thus, it makes more sense to allow embedders to specify that we don't want to register the ESM loader when creating an Environment via flag.

I also considered adding a new cli flag and just checking it via getOptionValue in pre_execution.js, but i think that (after chatting a bit with Anna) since this use case is quite specific to embedders that it makes more sense to make it an EnvironmentFlag. The only thing i wasn't sure of here was where specifically to expose it. I chose node_options but welcome any feedback or alternate preferences.

cc @addaleax

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@codebytere codebytere requested a review from addaleax June 26, 2020 01:22
@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 Jun 26, 2020
src/node.h Outdated Show resolved Hide resolved
@devsnek
Copy link
Member

devsnek commented Jun 26, 2020

Is it possible to add a test for this?

@codebytere codebytere force-pushed the allow-disabling-esm-loader branch 2 times, most recently from 2e1a311 to a5de41e Compare June 27, 2020 01:50
@codebytere
Copy link
Member Author

codebytere commented Jun 27, 2020

@devsnek @addaleax i added two tests - i couldn't figure out how to do it any more simply so feel free to suggest alternatives or ways to simplify!

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 27, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/32116/ (:yellow_heart:)

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. embedding Issues and PRs related to embedding Node.js in another project. labels Jun 27, 2020
codebytere added a commit that referenced this pull request Jun 29, 2020
PR-URL: #34060
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@codebytere
Copy link
Member Author

Landed in c23d2fd

@codebytere codebytere closed this Jun 29, 2020
@codebytere codebytere deleted the allow-disabling-esm-loader branch June 29, 2020 16:12
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
PR-URL: #34060
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MylesBorins added a commit that referenced this pull request Jul 14, 2020
Notable changes:

doc:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303

PR-URL: TODO
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins added a commit that referenced this pull request Jul 15, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 16, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
PR-URL: #34060
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MylesBorins added a commit that referenced this pull request Jul 16, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 20, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 20, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 20, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
MylesBorins added a commit that referenced this pull request Jul 21, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
Notable changes:

deps:
  * upgrade npm to 6.14.6 (claudiahdz) #34246
  * upgrade to libuv 1.38.1 (Colin Ihrig) #34187
  * (SEMVER-MINOR) update V8 to 8.4.371.19 (Michaël Zasso) [#33579](#33579)
module:
  * (SEMVER-MINOR) deprecate module.parent (Antoine du HAMEL) #32217
  * (SEMVER-MINOR) package "imports" field (Guy Bedford) #34117
src:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
  * (SEMVER-MINOR) allow embedders to disable esm loader (Shelley Vohr) #34060
tls:
  * (SEMVER-MINOR) make 'createSecureContext' honor more options (Mateusz Krawczuk) #33974
vm:
  * (SEMVER-MINOR) add run-after-evaluate microtask mode (Anna Henningsen) #34023
worker:
  * (SEMVER-MINOR) add option to track unmanaged file descriptors (Anna Henningsen) #34303
New Collaborators:
  * add danielleadams to collaborators (Danielle Adams) #34360
  * add sxa as collaborator (Stewart X Addison) #34338
  * add ruyadorno to collaborators (Ruy Adorno) #34297

PR-URL: #34371
addaleax pushed a commit that referenced this pull request Sep 23, 2020
PR-URL: #34060
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants