Skip to content

module: handle instantiated async module jobs in require(esm) #58067

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

Merged
merged 1 commit into from
May 8, 2025

Conversation

joyeecheung
Copy link
Member

When require(esm) encounters a cached module job that is instantiated but not yet evaluated, run the evaluation. This catches an edge case previously missed in #57187.

Fixes: #58061

When require(esm) encounters a cached module job that is instantiated
but not yet evaluated, run the evaluation. This catches an edge case
previously missed in nodejs#57187.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/vm

@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. needs-ci PRs that need a full CI run. labels Apr 29, 2025
Copy link

codecov bot commented Apr 29, 2025

Codecov Report

Attention: Patch coverage is 81.39535% with 8 lines in your changes missing coverage. Please review.

Project coverage is 90.21%. Comparing base (53abd1a) to head (da4207a).
Report is 117 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/modules/esm/module_job.js 79.41% 7 Missing ⚠️
src/module_wrap.cc 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #58067   +/-   ##
=======================================
  Coverage   90.20%   90.21%           
=======================================
  Files         630      630           
  Lines      186391   186423   +32     
  Branches    36612    36615    +3     
=======================================
+ Hits       168134   168177   +43     
+ Misses      11068    11061    -7     
+ Partials     7189     7185    -4     
Files with missing lines Coverage Δ
lib/internal/modules/esm/loader.js 96.07% <100.00%> (+1.02%) ⬆️
src/debug_utils.h 80.00% <ø> (ø)
src/module_wrap.h 0.00% <ø> (ø)
src/module_wrap.cc 72.38% <87.50%> (+0.02%) ⬆️
lib/internal/modules/esm/module_job.js 97.12% <79.41%> (-1.15%) ⬇️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2025
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label May 5, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 5, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/58067
✔  Done loading data for nodejs/node/pull/58067
----------------------------------- PR info ------------------------------------
Title      module: handle instantiated async module jobs in require(esm) (#58067)
Author     Joyee Cheung <[email protected]> (@joyeecheung)
Branch     joyeecheung:fix-instantiated -> nodejs:main
Labels     c++, lib / src, needs-ci
Commits    1
 - module: handle instantiated async module jobs in require(esm)
Committers 1
 - Joyee Cheung <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/58067
Fixes: https://github.com/nodejs/node/issues/58061
Reviewed-By: Jacob Smith <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/58067
Fixes: https://github.com/nodejs/node/issues/58061
Reviewed-By: Jacob Smith <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 29 Apr 2025 00:39:29 GMT
   ✔  Approvals: 1
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/58067#pullrequestreview-2802523383
   ✘  This PR needs to wait 7 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-05-05T10:51:08Z: https://ci.nodejs.org/job/node-test-pull-request/66609/
- Querying data for job/node-test-pull-request/66609/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/14841472093

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 8, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2025
@nodejs-github-bot nodejs-github-bot merged commit e0766f9 into nodejs:main May 8, 2025
85 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e0766f9

targos pushed a commit that referenced this pull request May 16, 2025
When require(esm) encounters a cached module job that is instantiated
but not yet evaluated, run the evaluation. This catches an edge case
previously missed in #57187.

PR-URL: #58067
Fixes: #58061
Reviewed-By: Jacob Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Cannot import mjs from cjs if this mjs file is already planned to be imported (ReferenceError: default is not defined)
3 participants