Skip to content

module: convert schema-only core module on convertCJSFilenameToURL #58612

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

himself65
Copy link
Member

Fixes: #58607

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Jun 7, 2025
@himself65 himself65 force-pushed the himself65/2025/06/07/custom-hook branch from 136b877 to 817052d Compare June 7, 2025 09:13
@joyeecheung
Copy link
Member

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).

Copy link

codecov bot commented Jun 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.14%. Comparing base (d6dade5) to head (4b27d6a).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58612      +/-   ##
==========================================
+ Coverage   90.12%   90.14%   +0.01%     
==========================================
  Files         637      637              
  Lines      188121   188125       +4     
  Branches    36892    36905      +13     
==========================================
+ Hits       169552   169593      +41     
+ Misses      11313    11260      -53     
- Partials     7256     7272      +16     
Files with missing lines Coverage Δ
lib/internal/modules/customization_hooks.js 100.00% <100.00%> (ø)

... and 30 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.

@himself65 himself65 force-pushed the himself65/2025/06/07/custom-hook branch from 22850d8 to a49f57b Compare June 7, 2025 10:57
@himself65
Copy link
Member Author

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 himself65 changed the title module: skip check builtin module on validateLoad module: fix schema only core module on convertCJSFilenameToURL Jun 7, 2025
@himself65 himself65 changed the title module: fix schema only core module on convertCJSFilenameToURL module: convert schema-only core module on convertCJSFilenameToURL Jun 7, 2025
@himself65 himself65 force-pushed the himself65/2025/06/07/custom-hook branch 3 times, most recently from 08ec6cc to d2b8035 Compare June 7, 2025 11:33
@himself65 himself65 force-pushed the himself65/2025/06/07/custom-hook branch from 2f30145 to 9bcd67a Compare June 9, 2025 16:10
@himself65 himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2025
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jun 9, 2025
Copy link
Contributor

github-actions bot commented Jun 9, 2025

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

@himself65 himself65 requested a review from joyeecheung June 11, 2025 16:02
@joyeecheung
Copy link
Member

#58612 (comment) @himself65 in case you missed it

@himself65 himself65 force-pushed the himself65/2025/06/07/custom-hook branch from 1bec580 to a79df89 Compare June 16, 2025 07:54
@himself65 himself65 force-pushed the himself65/2025/06/07/custom-hook branch from 628e6d7 to 4b27d6a Compare June 16, 2025 16:14
@joyeecheung joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Jun 19, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2025
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

schemeless builtins crash CJS loader
3 participants