-
Notifications
You must be signed in to change notification settings - Fork 216
Fix Node.js 22 strip-only loader regression in mocha setup #1740
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
Conversation
Updates import syntax in test files to be compatible with the new experimental TypeScript loader enabled by default in Node.js v22.18.0+. Starting in v22.18.0, Node.js enables a "strip-only" mode for TypeScript files by default. This new loader is stricter and does not support certain TypeScript-specific syntax, such as `import = require()` declarations, which were previously handled by a separate transpiler. This change caused `TypeError` and `SyntaxError` exceptions during test runs. This commit replaces the incompatible syntax with standard ES module imports to resolve the test failures.
Summary of ChangesHello @taeold, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical compatibility issues arising from the stricter experimental TypeScript loader introduced in Node.js v22.18.0 and later. The changes involve migrating specific import syntaxes within test files from the now-unsupported Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly updates the import syntax in test files to be compatible with Node.js v22, replacing TypeScript-specific import = require()
and non-standard namespace imports with modern ES module syntax. The changes are consistent and well-explained. I've just noted a minor issue regarding the removal of trailing newlines in two of the test files, which is a common convention to maintain.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses a regression in the mocha test setup caused by Node.js 22's new strip-only TypeScript loader. The solution to normalize CommonJS module exports for chai-as-promised
and nock
is robust, ensuring compatibility across different Node.js versions and loaders without broad configuration changes. The accompanying updates to sinon
imports in test files are also correct. The changes are well-implemented and I have no suggestions for improvement.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a regression with the TypeScript loader in Node.js 22 by normalizing CommonJS module imports for chai-as-promised
and nock
. It also modernizes the codebase by replacing js-yaml
with the ESM-native yaml
package and updating sinon
imports to use ES module syntax.
The changes are well-targeted and address the compatibility issue effectively. I've made one suggestion in mocha/setup.ts
to improve the consistency and readability of the import shims, making them easier to understand and maintain. Overall, this is a solid improvement.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! I hadn't realized type stripping was enabled by default
Summary
Node.js 22 enables the strip-only TypeScript loader by default, which no longer auto-wraps CommonJS exports the way Node.js 20's ts-node did. We normalize the mocha setup imports for
chai-as-promised
andnock
, mirror that shim in the bin test harness, and swap the manifest parser fromjs-yaml
to the native ESMyaml
package so no ad-hoc interop is needed.Testing