FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: allow embedders to disable esm loader by codebytere · Pull Request #34060 · nodejs/node · GitHub

/ node Public

src: allow embedders to disable esm loader - #34060

Closed
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:allow-disabling-esm-loader
ConversationCommits (1)ChecksFiles changed
Closed

src: allow embedders to disable esm loader#34060
codebytere wants to merge 1 commit into
nodejs:masterfrom
codebytere:allow-disabling-esm-loader

Conversation

codebytere commented Jun 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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 requested a review from addaleax June 26, 2020 01:22
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
richardlau reviewed Jun 26, 2020
View reviewed changes
Comment thread src/node.h Outdated

devsnek commented Jun 26, 2020

Copy link
Copy Markdown
Member

Is it possible to add a test for this?

codebytere force-pushed the allow-disabling-esm-loader branch 2 times, most recently from 2e1a311 to a5de41e Compare June 27, 2020 01:50
codebytere force-pushed the allow-disabling-esm-loader branch from a5de41e to 40acad0 Compare June 27, 2020 02:20

codebytere commented Jun 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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 commented Jun 27, 2020 •
edited by addaleax
Loading

Copy link
Copy Markdown
Collaborator

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

addaleax added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. embedding Issues and PRs related to embedding Node.js in another project. labels Jun 27, 2020
codebytere mentioned this pull request Jun 29, 2020
Merged
codebytere added a commit that referenced this pull request Jun 29, 2020
src: allow embedders to disable esm loader
c23d2fd 
PR-URL: #34060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>

codebytere commented Jun 29, 2020

Copy link
Copy Markdown
Member Author

Landed in c23d2fd

codebytere closed this Jun 29, 2020
codebytere deleted the allow-disabling-esm-loader branch June 29, 2020 16:12
codebytere mentioned this pull request Jun 29, 2020
Closed
3 tasks
addaleax mentioned this pull request Jul 14, 2020
Closed
4 tasks
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
src: allow embedders to disable esm loader
74e30ee 
PR-URL: #34060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
MylesBorins added a commit that referenced this pull request Jul 14, 2020
2020-07-15, Version 14.6.0 (Current)
2e91d52 
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 mentioned this pull request Jul 14, 2020
Merged
MylesBorins added a commit that referenced this pull request Jul 15, 2020
2020-07-16, Version 14.6.0 (Current)
6629373 
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
2020-07-16, Version 14.6.0 (Current)
93694f6 
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
src: allow embedders to disable esm loader
e4c7b59 
PR-URL: #34060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
MylesBorins added a commit that referenced this pull request Jul 16, 2020
2020-07-16, Version 14.6.0 (Current)
35a4b7b 
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
2020-07-21, Version 14.6.0 (Current)
049631f 
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
2020-07-21, Version 14.6.0 (Current)
7999aa2 
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
2020-07-21, Version 14.6.0 (Current)
ca62c34 
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
2020-07-21, Version 14.6.0 (Current)
a981c65 
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
2020-07-21, Version 14.6.0 (Current)
38678ef 
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
src: allow embedders to disable esm loader
5e28660 
PR-URL: #34060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
codebytere mentioned this pull request Sep 28, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

richardlau richardlau left review comments
jasnell jasnell approved these changes
addaleax addaleax approved these changes
+2 more reviewers
devnexen devnexen approved these changes
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL