Skip to content

module: refactor commonjs typescript loader - #58657

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
marco-ippolito:remove-ts-from-require-extensions
Jun 12, 2025
Merged

module: refactor commonjs typescript loader#58657
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
marco-ippolito:remove-ts-from-require-extensions

Conversation

@marco-ippolito

@marco-ippolitomarco-ippolito commented Jun 10, 2025

Copy link
Copy Markdown
Member

Refs: nodejs/typescript#37

As suggested by @joyeecheungnodejs/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 @hardfisthttps://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
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-botnodejs-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-ippolitomarco-ippolito added the strip-types Issues or PRs related to strip-types support label Jun 10, 2025
@codecov

codecovBot commented Jun 10, 2025

Copy link
Copy Markdown

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 142 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/modules/cjs/loader.js78.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 linesCoverage Δ
lib/internal/modules/cjs/loader.js97.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.

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@marco-ippolitomarco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@marco-ippolitomarco-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
Copy Markdown
Collaborator

Comment threadlib/internal/modules/cjs/loader.js Outdated
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
@marco-ippolitomarco-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-ippolitomarco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

hardfist commented Jun 11, 2025

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
MemberAuthor

v24 yes in the next release
v23 no since is eol

@nodejs-github-botnodejs-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:mainJun 12, 2025
@nodejs-github-bot

Copy link
Copy Markdown
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>
@JLHwung

Copy link
Copy Markdown
Contributor

With this change, how should a library detect if the current node has enabled builtin strip-types support? Should a library do the version check?

Previously in Babel we checked require.extensions (set by ts-node, tsx or node < 24.3) and nudged users to upgrade Node or install the aforementioned libraries if they want to use a TS Babel config file such as babel.config.cts.

@marco-ippolito

marco-ippolito commented Jun 23, 2025

Copy link
Copy Markdown
MemberAuthor

With this change, how should a library detect if the current node has enabled builtin strip-types support? Should a library do the version check?

Previously in Babel we checked require.extensions (set by ts-node, tsx or node < 24.3) and nudged users to upgrade Node or install the aforementioned libraries if they want to use a TS Babel config file such as babel.config.cts.

process.features.typescript
https://nodejs.org/api/process.html#processfeaturestypescript

@marco-ippolitomarco-ippolito added backport-open-v22.x Indicate that the PR has an open backport dont-land-on-v20.x labels Jun 25, 2025
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Jul 3, 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: nodejs#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>
marco-ippolito added a commit that referenced this pull request Jul 4, 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
Backport-PR-URL: #57298
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>
PR-URL: #56350Fixes: nodejs/typescript#17
@marco-ippolitomarco-ippolito added backported-to-v22.x PRs backported to the v22.x-staging branch. and removed backport-open-v22.x Indicate that the PR has an open backport labels Jul 4, 2025
RafaelGSS pushed a commit that referenced this pull request Jul 15, 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
Backport-PR-URL: #57298
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>
PR-URL: #56350Fixes: nodejs/typescript#17
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.backported-to-v22.xPRs backported to the v22.x-staging branch.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.moduleIssues and PRs related to the module subsystem.needs-ciPRs that need a full CI run.strip-typesIssues or PRs related to strip-types support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@marco-ippolito@nodejs-github-bot@hardfist@JLHwung@mcollina@joyeecheung@meixg@pmarchini