node icon indicating copy to clipboard operation
node copied to clipboard

module: convert schema-only core module on `convertCJSFilenameToURL`

Open himself65 opened this issue 6 months ago • 6 comments

Fixes: https://github.com/nodejs/node/issues/58607

himself65 avatar Jun 07 '25 09:06 himself65

Review requested:

  • [ ] @nodejs/loaders

nodejs-github-bot avatar Jun 07 '25 09:06 nodejs-github-bot

From the issue description I don’t think its the validation that should be changed - it should be the URL that gets passed into the hooks that should be corrected (it should’ve been node:sea instead of sea, looks like somewhere in the CJS loader the conversion is missed since internally we use filenames and ids everywhere and only convert them to URLs when being passed into hooks).

joyeecheung avatar Jun 07 '25 09:06 joyeecheung

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.10%. Comparing base (faada65) to head (ee2dc06). Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #58612   +/-   ##
=======================================
  Coverage   90.09%   90.10%           
=======================================
  Files         640      640           
  Lines      188271   188275    +4     
  Branches    36923    36923           
=======================================
+ Hits       169625   169641   +16     
+ Misses      11386    11344   -42     
- Partials     7260     7290   +30     
Files with missing lines Coverage Δ
lib/internal/modules/customization_hooks.js 100.00% <100.00%> (ø)

... and 34 files with indirect coverage changes

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

codecov[bot] avatar Jun 07 '25 10:06 codecov[bot]

From the issue description I don’t think its the validation that should be changed - it should be the URL that gets passed into the hooks that should be corrected (it should’ve been node:sea instead of sea, looks like somewhere in the CJS loader the conversion is missed since internally we use filenames and ids everywhere and only convert them to URLs when being passed into hooks).

Yeah, I read the code and think the issue should be in convertCJSFilenameToURL ? updated the code now

himself65 avatar Jun 07 '25 10:06 himself65

Failed to start CI
   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/15542291850

github-actions[bot] avatar Jun 09 '25 18:06 github-actions[bot]

https://github.com/nodejs/node/pull/58612#discussion_r2137646849 @himself65 in case you missed it

joyeecheung avatar Jun 15 '25 13:06 joyeecheung

CI: https://ci.nodejs.org/job/node-test-pull-request/67545/

nodejs-github-bot avatar Jun 19 '25 20:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/67634/

nodejs-github-bot avatar Jun 24 '25 09:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/67635/

nodejs-github-bot avatar Jun 24 '25 10:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/67636/

nodejs-github-bot avatar Jun 24 '25 11:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/67639/

nodejs-github-bot avatar Jun 24 '25 16:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/67648/

nodejs-github-bot avatar Jun 25 '25 11:06 nodejs-github-bot

Landed in a705e240b197262d5a8eee39f11797d14cfc94cb

nodejs-github-bot avatar Jun 25 '25 16:06 nodejs-github-bot