Skip to content

module: refactor commonjs typescript loader #58657

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

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Jun 10, 2025

Refs: nodejs/typescript#37

As suggested by @joyeecheung nodejs/typescript#37 (comment), we can refactor the commonjs loader so that we dont have to expose in the Module._extensions the TypeScript file extensions.

This also means that require.extensions will be inconsistent but it already is since .cjs and .mjs are not exposed

This should unblock the backport of Type Stripping in Node v22.

I tested it against @hardfist https://github.com/hardfist/webpack-ts-break and it fixes the breaking change:

Node v24
marcoippolito@marcos-MacBook-Pro-3 webpack-ts-break % node  node_modules/webpack/bin/webpack.js
[webpack-cli] Failed to load '/Users/marcoippolito/Documents/projects/test/webpack-ts-break/webpack.config.ts' config
[webpack-cli] ReferenceError: __dirname is not defined
    at /Users/marcoippolito/Documents/projects/test/webpack-ts-break/webpack.config.ts:5:12
    at ModuleJobSync.runSync (node:internal/modules/esm/module_job:455:35)
    at ModuleLoader.importSyncForRequire (node:internal/modules/esm/loader:435:47)
    at loadESMFromCJS (node:internal/modules/cjs/loader:1565:24)
    at Module._compile (node:internal/modules/cjs/loader:1716:5)
    at Object.loadTS [as .ts] (node:internal/modules/cjs/loader:1826:10)
    at Module.load (node:internal/modules/cjs/loader:1469:32)
    at Module._load (node:internal/modules/cjs/loader:1286:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)
    
    
This PR
marcoippolito@marcos-MacBook-Pro-3 webpack-ts-break % ../../forks/node/out/Release/node node_modules/webpack/bin/webpack.js
asset main.js 21 bytes [emitted] [minimized] (name: main)
./src/index.js 20 bytes [built] [code generated]
webpack 5.99.9 compiled successfully in 100 ms

This commit refactors the CommonJS loader to remove TypeScript-specific
extensions from the require.extensions object for compatibility with
libraries that depended on it to initialize extenal TypeScript loaders.
@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 10, 2025
@marco-ippolito marco-ippolito added the strip-types Issues or PRs related to strip-types support label Jun 10, 2025
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.15%. Comparing base (a45f1ad) to head (c942d9f).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/modules/cjs/loader.js 78.94% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #58657   +/-   ##
=======================================
  Coverage   90.14%   90.15%           
=======================================
  Files         636      636           
  Lines      188030   187953   -77     
  Branches    36894    36882   -12     
=======================================
- Hits       169506   169451   -55     
+ Misses      11276    11268    -8     
+ Partials     7248     7234   -14     
Files with missing lines Coverage Δ
lib/internal/modules/cjs/loader.js 97.85% <78.94%> (+0.06%) ⬆️

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

@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

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2025
@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito marco-ippolito added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 10, 2025
@nodejs-github-bot
Copy link
Collaborator

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
@marco-ippolito marco-ippolito added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jun 10, 2025
@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2025
@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2025
@hardfist
Copy link
Contributor

hardfist commented Jun 11, 2025

@marco-ippolito great job! may I ask whether this compatible fix be backported to Node.js 23&24? we're willing to migrate to native typescript support but a easier progressive migration is always preferred.

@marco-ippolito
Copy link
Member Author

v24 yes in the next release
v23 no since is eol

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 12, 2025
@nodejs-github-bot nodejs-github-bot merged commit 708477b into nodejs:main Jun 12, 2025
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 708477b

targos pushed a commit that referenced this pull request Jun 16, 2025
This commit refactors the CommonJS loader to remove TypeScript-specific
extensions from the require.extensions object for compatibility with
libraries that depended on it to initialize extenal TypeScript loaders.

PR-URL: #58657
Refs: nodejs/typescript#37
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants