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

lib: always initialize esm loader callbackMap #34127

Closed
wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jun 29, 2020

Refs #34060.

We should instead unilaterally initialize the callbackMap and return early if an embedder is choosing to not use the Node.js esm loader is in an embedder context.

I also updated the corresponding tests to more directly check importModuleDynamically functionality affected by a decision to not use the esm loader provided by Node.js.

cc @addaleax @joyeecheung

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 29, 2020
@nodejs-github-bot
Copy link
Collaborator

@codebytere codebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Jun 30, 2020
@addaleax
Copy link
Member

addaleax commented Jul 1, 2020

Would it maybe make more sense to provide callbackMap unconditionally? e.g. this (untested):

diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js
index a51dbf05ec4f..8d4c44922ac0 100644
--- a/lib/internal/bootstrap/pre_execution.js
+++ b/lib/internal/bootstrap/pre_execution.js
@@ -60,9 +60,7 @@ function prepareMainThreadExecution(expandArgv1 = false) {
   initializeWASI();
   initializeCJSLoader();
 
-  if (!shouldNotRegisterESMLoader) {
-    initializeESMLoader();
-  }
+  initializeESMLoader();
 
   const CJSLoader = require('internal/modules/cjs/loader');
   assert(!CJSLoader.hasLoadedAnyUserCJSModule);
@@ -406,6 +404,8 @@ function initializeESMLoader() {
   // Create this WeakMap in js-land because V8 has no C++ API for WeakMap.
   internalBinding('module_wrap').callbackMap = new SafeWeakMap();
 
+  if (!shouldNotRegisterESMLoader) return;
+
   const {
     setImportModuleDynamicallyCallback,
     setInitializeImportMetaObjectCallback

That requires a smaller patch and would also make things easier in case we do figure out a good way to mix ESM loaders (e.g. by making them per-Context)?

@codebytere
Copy link
Member Author

That also works! Will do :)

@codebytere codebytere changed the title lib: handle missing callbackMap in esm logic lib: always initialize esm loader callbackMap Jul 1, 2020
@nodejs-github-bot
Copy link
Collaborator

@codebytere
Copy link
Member Author

Landed in 37fc587

@codebytere codebytere closed this Jul 3, 2020
codebytere added a commit that referenced this pull request Jul 3, 2020
PR-URL: #34127
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
@codebytere codebytere deleted the move-callback-map branch July 3, 2020 21:32
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
PR-URL: #34127
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
PR-URL: #34127
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
addaleax pushed a commit that referenced this pull request Sep 23, 2020
PR-URL: #34127
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
@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
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants