Skip to content

fix: read ambigous TS files with type-stripping enabled with require first#1260

Open
shadowusr wants to merge 1 commit into
masterfrom
users/shadowusr/INFRADUTY-30280
Open

fix: read ambigous TS files with type-stripping enabled with require first#1260
shadowusr wants to merge 1 commit into
masterfrom
users/shadowusr/INFRADUTY-30280

Conversation

@shadowusr
Copy link
Copy Markdown
Member

What's done?

Before, we had the following issue when node's built in typescript types stripping (https://nodejs.org/download/release/v24.1.0/docs/api/typescript.html#type-stripping) is enabled:

  1. Mocha tries to read tests with readFilesAsync, which import()s them first.
  2. Node strips types and tries to read file as cjs (since root package json is not type: module)
  3. Node fails and sees imports in typescript file and tries to re-read the file as ESM, issuing the warning MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of some-file.hermione.ts?rand=0.4001007512274388 is not specified and it doesn't parse as CommonJS. Reparsing as ES module because module syntax was detected
  4. This fails too, finally mocha reads it with require, our transform hooks apply and file reads fine.

This means 3 file reads and 1 warning in console.

This PR adds the following logic:

  • If node's type stripping is active and file is typescript with ambigous extension (not cts/mts), we first try to read it with loadFile (which uses require) and secondly if it fails, loadFileAsync.

Also added 4 test projects that test file reading under different circumstances e2e.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 21, 2026

Open in StackBlitz

npm i https://pkg.pr.new/gemini-testing/testplane@1260

commit: ca5e4f9

Copy link
Copy Markdown
Member

@KuznetsovRoman KuznetsovRoman left a comment

Choose a reason for hiding this comment

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

🔥

Comment on lines +16 to +20
- name: Use Node.js 24.x
uses: actions/setup-node@v4
with:
node-version: 24.x
cache: "npm"
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.

I think we should run another node.js versions also: like "20" and "22"

If you checked it once and it works on 18/20/22 - its fine, we can keep "only node 24" check

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ESM wouldn't work on node < v20.17.0, and only 24 has typescript types stripping turned on by default. I'll verify once again locally on different node versions, but going to ultimately keep the check turned on for 24 only

Comment thread src/test-reader/mocha-reader/index.js
Comment on lines +94 to +100
for (const file of files) {
mocha.files = [file];

if (!shouldLoadWithRequire(file)) {
await mocha.loadFilesAsync({ esmDecorator });
continue;
}
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.

Why isn't it written like that?

const loadWithRequireFiles = [];
const loadDefaultFiles = [];

for (const file of files) {
    if (shouldLoadWithRequire(file)) {
        loadWithRequireFiles.push(file)
    } else {
        loadDefaultFiles.push(file)
    }
}

// Load "loadDefaultFiles" with some kind of Promise.all
// Load "loadWithRequireFiles" with "mocha.loadFiles"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We wouldn't gain anything from doing that: mocha itself loads files sequentially, here's the source of loadFilesAsync:

exports.loadFilesAsync = async (
  files,
  preLoadFunc,
  postLoadFunc,
  esmDecorator
) => {
  for (const file of files) {
    preLoadFunc(file);
    const result = await exports.requireOrImport(
      path.resolve(file),
      esmDecorator
    );
    postLoadFunc(file, result);
  }
};

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants