Skip to content

Conversation

@codebytere
Copy link
Member

@codebyterecodebytere 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-botnodejs-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

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

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
MemberAuthor

That also works! Will do :)

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

@codebytere
Copy link
MemberAuthor

Landed in 37fc587

codebytere added a commit that referenced this pull request Jul 3, 2020
PR-URL: #34127 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
@codebyterecodebytere 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 <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
@MylesBorinsMylesBorins 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 <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 23, 2020
PR-URL: #34127 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
@codebyterecodebytere mentioned this pull request Sep 28, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

embeddingIssues and PRs related to embedding Node.js in another project.lib / srcIssues 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.

4 participants

@codebytere@nodejs-github-bot@addaleax@devsnek