-
Notifications
You must be signed in to change notification settings - Fork 142
Additional config changes #850
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
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Size Change: 0 B Total Size: 3.51 MB ℹ️ View Unchanged
|
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.
Pull request overview
This pull request prepares the codebase for ESM (ECMAScript Module) migration by enforcing that TypeScript source files use .js extensions on relative imports. The changes ensure compatibility with ESM while the project is still using CommonJS and transitioning to TypeScript.
Changes:
- Adds ESLint rule to enforce
.jsextensions on all TypeScript relative imports - Configures build tools (webpack) and test infrastructure to resolve
.jsimports to.tsfiles - Adds VS Code workspace settings for consistent tooling configuration
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-helper.js | Patches Node.js module resolution to allow .js imports to resolve to .ts files during testing |
| pnpm-lock.yaml | Adds eslint-plugin-import-x@4.16.1 and its transitive dependencies (including unrs-resolver with platform-specific native bindings) |
| package.json | Adds eslint-plugin-import-x as a devDependency |
| config/webpack.config.browser.js | Configures webpack extensionAlias to resolve .js imports to .ts or .js files in order of preference |
| config/eslint.config.cjs | Adds import-x/extensions rule to enforce .js extensions on all TypeScript relative imports (ignoring package imports) |
| .vscode/settings.json | Configures VS Code to use the project's ESLint and Prettier configuration files |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Module._resolveFilename = function (request, parent, ...args) { | ||
| try { | ||
| return originalResolveFilename.call(this, request, parent, ...args); | ||
| } catch (err) { | ||
| if (request.endsWith(".js")) { | ||
| return originalResolveFilename.call( | ||
| this, | ||
| request.replace(/\.js$/, ".ts"), | ||
| parent, | ||
| ...args | ||
| ); | ||
| } | ||
| throw err; | ||
| } |
Copilot
AI
Feb 10, 2026
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.
The module resolution patch only handles the case when a .js file doesn't exist and tries to resolve to .ts. However, this approach will catch and swallow ANY error that occurs during the first resolution attempt, not just "file not found" errors. This could hide legitimate errors like circular dependencies, syntax errors in module resolution, or permission issues. Consider checking the error code to ensure it's specifically a MODULE_NOT_FOUND error before attempting the .ts fallback.
| if (request.endsWith(".js")) { | ||
| return originalResolveFilename.call( | ||
| this, | ||
| request.replace(/\.js$/, ".ts"), | ||
| parent, | ||
| ...args | ||
| ); |
Copilot
AI
Feb 10, 2026
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.
The module resolution patch will also affect all .js imports throughout the test suite, not just relative imports. This means if any third-party package is imported with a .js extension that doesn't exist, it will try to resolve to a .ts file, which could lead to unexpected behavior or confusing error messages. Consider adding a check to only apply the patch to relative imports (those starting with . or ..) by checking if request starts with . before attempting the .ts resolution.
| extensionAlias: { | ||
| ".js": [".ts", ".js"] | ||
| } |
Copilot
AI
Feb 10, 2026
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.
The extensionAlias configuration maps .js to [".ts", ".js"], which means webpack will try .ts first, then .js. However, this could cause confusion during development if both a .ts and .js file with the same name exist in the same directory, as webpack will always prefer the .ts file. While this is unlikely in a well-organized codebase, consider documenting this behavior or adding validation to prevent such scenarios.
| return originalResolveFilename.call( | ||
| this, | ||
| request.replace(/\.js$/, ".ts"), | ||
| parent, | ||
| ...args | ||
| ); |
Copilot
AI
Feb 10, 2026
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.
When the module resolution patch attempts to resolve the .ts file (line 9-14), if that also fails, the error thrown will be about the .ts file not being found, not the original .js file. This could make debugging more confusing. Consider catching the second error and re-throwing the original error with additional context about the attempted .ts resolution.
| return originalResolveFilename.call( | |
| this, | |
| request.replace(/\.js$/, ".ts"), | |
| parent, | |
| ...args | |
| ); | |
| const originalError = err; | |
| const tsRequest = request.replace(/\.js$/, ".ts"); | |
| try { | |
| return originalResolveFilename.call( | |
| this, | |
| tsRequest, | |
| parent, | |
| ...args | |
| ); | |
| } catch (tsErr) { | |
| originalError.message += | |
| ` (and failed to resolve corresponding TypeScript file "${tsRequest}" as well: ${tsErr.message})`; | |
| throw originalError; | |
| } |
quietbits
left a comment
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.
Is this something we'll need to keep after the migration is done?
.vscode/settings.json
Outdated
| @@ -0,0 +1,7 @@ | |||
| { | |||
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.
How will this work with existing .vscode settings locally?
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.
Hmm Im not sure about I initially wanted to commit this because of where the configs are stored the editor fails to pick them up by default. In hindsight though I think this folder is generally git ignored.
|
This is module resolution changes in the |
987facb to
0b16894
Compare
.json relative imports. As we migrate I think it'd save time if we imported with ESM compatibility.