Skip to content

Conversation

@joyeecheung
Copy link
Member

For detecting whether require(esm) is supported without triggering the experimental warning.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added 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 Oct 2, 2024
@codecov
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (bbdfeeb) to head (1609e78).
Report is 845 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55241      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         652      652              
  Lines      186612   186615       +3     
  Branches    36062    36061       -1     
==========================================
+ Hits       165001   165003       +2     
+ Misses      14885    14879       -6     
- Partials     6726     6733       +7     
Files with missing lines Coverage Δ
lib/internal/bootstrap/node.js 99.78% <100.00%> (+<0.01%) ⬆️

... and 34 files with indirect coverage changes

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@richardlau
Copy link
Member

test-process-features will need an update:

Details

https://github.com/nodejs/node/actions/runs/11147823375/job/30983051322?pr=55241#step:7:6106

=== release test-process-features ===
Path: parallel/test-process-features
Error: --- stderr ---
node:assert:90
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

+ Set(10) {
- Set(9) {
    'cached_builtins',
    'debug',
    'inspector',
    'ipv6',
+   'require_module',
    'tls',
...
    'tls_sni',
    'uv'
  }
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-process-features.js:8:8)
    at Module._compile (node:internal/modules/cjs/loader:1560:14)
    at Object..js (node:internal/modules/cjs/loader:1703:10)
    at Module.load (node:internal/modules/cjs/loader:1328:32)
    at Function._load (node:internal/modules/cjs/loader:1138:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:218:24)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:170:5)
    at node:internal/main/run_main_module:36:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: Set(10) {
    'inspector',
    'debug',
    'uv',
    'ipv6',
    'tls_alpn',
    'tls_sni',
    'tls_ocsp',
    'tls',
    'cached_builtins',
    'require_module'
  },
  expected: Set(9) {
    'inspector',
    'debug',
    'uv',
    'ipv6',
    'tls_alpn',
    'tls_sni',
    'tls_ocsp',
    'tls',
    'cached_builtins'
  },
  operator: 'deepStrictEqual'
}

Node.js v23.0.0-pre
Command: out/Release/node --test-reporter=spec --test-reporter-destination=stdout --test-reporter=./tools/github_reporter/index.js --test-reporter-destination=stdout /home/runner/work/node/node/test/parallel/test-process-features.js

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

@richardlau richardlau added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 4, 2024
For detecting whether `require(esm)` is supported without triggering
the experimental warning.
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung force-pushed the require-module-detect branch from a40f0ad to 1609e78 Compare October 5, 2024 18:40
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

@aduh95 @mcollina @richardlau CI is green - can I get some re-approvals to help it make it into v23?

As for #55241 (comment) - I don't think the request to repeat the documentation with a false condition is blocking. Feel free to open a follow-up PR if you feel the addition is necessary. @aduh95

@joyeecheung joyeecheung mentioned this pull request Oct 7, 2024
9 tasks
@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 7, 2024
@joyeecheung joyeecheung added the backport-open-v20.x Indicate that the PR has an open backport label Feb 6, 2025
@joyeecheung
Copy link
Member Author

Backport in #56927

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-open-v20.x Indicate that the PR has an open backport commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. 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.

5 participants