fix: ignore js file when the same ts file exists #280
Conversation
WalkthroughThe pull request introduces comprehensive TypeScript support for the Egg.js framework by adding a new TypeScript-based test fixture project Changes
Sequence DiagramsequenceDiagram
participant FL as FileLoader
participant TS as TypeScript File
participant JS as JavaScript File
FL->>TS: Check for .ts file
alt .ts file exists
FL->>JS: Ignore .js file
FL-->>TS: Load TypeScript file
else No .ts file
FL-->>JS: Load JavaScript file
end
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #280 +/- ##
=======================================
Coverage 97.86% 97.86%
=======================================
Files 10 10
Lines 3366 3373 +7
Branches 591 594 +3
=======================================
+ Hits 3294 3301 +7
Misses 72 72 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
test/fixtures/helloworld-ts/config/config.default.js (1)
1-1: Remove redundant "use strict" directiveThe "use strict" directive is redundant in ES modules as they are automatically in strict mode.
-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/helloworld-ts/app/router.js (1)
1-1: Remove the redundant 'use strict' directive.
Modules automatically run in strict mode in modern JS environments, making this directive unnecessary.Apply the following diff:
-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/helloworld-ts/app/controller/foo.js (1)
1-1: Remove redundant 'use strict' directive.JavaScript modules are automatically executed in strict mode, making the
'use strict'directive unnecessary.-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
src/loader/file_loader.ts (1)
188-194: Ignore .js when .ts exists: validate side-effects.Automatically skipping the
.jsfile if a corresponding.tsfile is found aligns with TypeScript-first logic. However, be mindful of potential edge cases where only a.d.tsfile is present, or where the.jsfile may be necessary for other reasons. Consider logging a warning if ignoring.jsmight introduce unexpected runtime behaviors.package.json (1)
18-18: Consider removing redundant test-local scriptBoth
testandtest-localscripts now use the same commandegg-bin test. This duplication might cause confusion. Consider removing thetest-localscript if it's no longer serving a different purpose."scripts": { "test": "egg-bin test", "posttest": "npm run clean", - "test-local": "egg-bin test", "preci": "npm run clean && npm run lint && npm run prepublishOnly",Also applies to: 21-21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
package.json(1 hunks)src/loader/file_loader.ts(2 hunks)test/fixtures/helloworld-ts/.eslintrc(1 hunks)test/fixtures/helloworld-ts/.gitignore(1 hunks)test/fixtures/helloworld-ts/app/controller/foo.js(1 hunks)test/fixtures/helloworld-ts/app/controller/foo.ts(1 hunks)test/fixtures/helloworld-ts/app/router.js(1 hunks)test/fixtures/helloworld-ts/app/router.ts(1 hunks)test/fixtures/helloworld-ts/config/config.default.js(1 hunks)test/fixtures/helloworld-ts/config/config.default.ts(1 hunks)test/fixtures/helloworld-ts/package.json(1 hunks)test/fixtures/helloworld-ts/test/index.test.ts(1 hunks)test/fixtures/helloworld-ts/tsconfig.json(1 hunks)test/support-typescript.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- test/fixtures/helloworld-ts/config/config.default.ts
- test/fixtures/helloworld-ts/.gitignore
- test/fixtures/helloworld-ts/.eslintrc
- test/fixtures/helloworld-ts/tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
test/fixtures/helloworld-ts/app/router.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/helloworld-ts/app/controller/foo.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/helloworld-ts/config/config.default.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (12)
test/fixtures/helloworld-ts/config/config.default.js (1)
1-5:
Remove this JS file since a TS version exists
According to the PR objective to "ignore js file when the same ts file exists", this JavaScript file should be removed since there's an identical TypeScript version at config/config.default.ts. Having both files maintained in parallel could lead to synchronization issues.
Let's verify the existence and content of the TypeScript version:
✅ Verification successful
Remove the redundant JavaScript config file
The TypeScript version of config.default.ts exists and contains identical configuration. As per the PR's objective to ignore JS files when corresponding TS files exist, this JavaScript file should be removed to avoid maintenance overhead and potential synchronization issues.
test/fixtures/helloworld-ts/config/config.default.jsshould be deleted as it's redundant withtest/fixtures/helloworld-ts/config/config.default.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the TS file exists and compare contents
if [ -f "test/fixtures/helloworld-ts/config/config.default.ts" ]; then
echo "TypeScript version exists. Comparing contents:"
diff <(cat test/fixtures/helloworld-ts/config/config.default.js) <(cat test/fixtures/helloworld-ts/config/config.default.ts)
else
echo "TypeScript version not found"
fi
Length of output: 405
Script:
#!/bin/bash
# Let's verify if the TS file exists first
fd "config.default.ts" test/fixtures/helloworld-ts/
# If it exists, let's see its content
if [ -f "test/fixtures/helloworld-ts/config/config.default.ts" ]; then
echo "Content of TS file:"
cat test/fixtures/helloworld-ts/config/config.default.ts
fi
# Let's also check if there are any other config files in this directory
echo -e "\nAll config files in the directory:"
fd "config" test/fixtures/helloworld-ts/config/
Length of output: 563
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/helloworld-ts/app/router.ts (1)
1-5: Looks good for a basic route setup.
You have a straightforward configuration for the root path. Everything appears correct and consistent.
test/fixtures/helloworld-ts/app/router.js (1)
3-7: Approved route logic.
The route logic matches the TypeScript counterpart.
test/fixtures/helloworld-ts/test/index.test.ts (2)
1-9: Tests for the root path look good.
They provide complete coverage for the "Hello World" scenario.
11-18: Validate that /foo behaves as expected in TypeScript.
Currently, the test checks the JavaScript controller. If the TypeScript controller is intended to deliver the same response, ensure the render method has a parallel implementation.
test/support-typescript.test.ts (1)
1-22: Looks good.
This new test file cleanly demonstrates the intended behavior of ignoring the .js file when a .ts file is present. The test case is straightforward, and the use of supertest for HTTP assertions is appropriate. Consider adding coverage for edge cases (such as when no .ts file exists) if needed by other requirements.
test/fixtures/helloworld-ts/app/controller/foo.js (1)
3-11: Controller logic looks sound, but verify context injection.
The controller references this.ctx, which is typically set by the Egg.js framework. Confirm that the context is properly injected in all relevant scenarios. If no injection is configured for this code path, errors may occur due to this.ctx being undefined.
src/loader/file_loader.ts (1)
9-9: Updated debug log message style is fine.
Switching from @eggjs/core:file_loader to @eggjs/core/file_loader is acceptable and more consistent with typical naming styles. No issues found.
test/fixtures/helloworld-ts/package.json (2)
4-5: Confirm stability of "egg" at version "beta".
Depending on your release cycle, you might prefer a pinned stable version to avoid unexpected breaking changes. Verify that this particular beta version is suitable for production or fully tested in your environment.
16-26: Script definitions appear comprehensive.
Scripts cover development, testing, linting, and build steps. Validate that these commands align with your CI/CD flow and do not introduce duplications. For instance, you might unify the lint steps under one lifecycle script if that better suits your project’s pipeline organization.
package.json (2)
19-19: LGTM! Clean post-hooks implementation
The addition of posttest and postci scripts ensures consistent cleanup after test and CI runs. This is a good practice for maintaining a clean state between test runs.
Also applies to: 23-23
Line range hint 1-108: Verify the main PR implementation
While these package.json changes support TypeScript integration, the main PR objective of ignoring JS files when equivalent TS files exist should be implemented elsewhere in the codebase.
Let's verify where this logic is implemented:
✅ Verification successful
Let's search more specifically for the implementation:
Found the TypeScript precedence implementation
The main PR objective of ignoring JS files when equivalent TS files exist is implemented in src/loader/file_loader.ts. The implementation includes:
- TypeScript file detection:
files = (process.env.EGG_TYPESCRIPT === 'true' && utils.extensions['.ts'])
? [ '**/*.(js|ts)', '!**/*.d.ts' ]
: [ '**/*.js' ];- JS file skipping when TS exists:
if (filepath.endsWith('.js')) {
const filepathTs = filepath.replace(/\.js$/, '.ts');
if (filepaths.includes(filepathTs)) {
debug('[parse] ignore %s, because %s exists', fullpath, filepathTs);
continue;
}
}Let's verify the utils extension configuration:
Implementation verified and complete
The PR's main objective of ignoring JS files when equivalent TS files exist is properly implemented in src/loader/file_loader.ts. The implementation consists of two main parts:
- File pattern selection based on TypeScript environment:
files = (process.env.EGG_TYPESCRIPT === 'true' && utils.extensions['.ts'])
? [ '**/*.(js|ts)', '!**/*.d.ts' ]
: [ '**/*.js' ];- JS file skipping logic when TS exists:
if (filepath.endsWith('.js')) {
const filepathTs = filepath.replace(/\.js$/, '.ts');
if (filepaths.includes(filepathTs)) {
debug('[parse] ignore %s, because %s exists', fullpath, filepathTs);
continue;
}
}The implementation uses Node.js's native module extensions (accessed via utils.extensions) and environment variable EGG_TYPESCRIPT to control TypeScript support, ensuring JS files are properly ignored when equivalent TS files exist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential file loading/extension handling logic
rg -g '!*.{json,md}' -l "\.ts|\.js"
# Search for specific loader or file resolution logic
ast-grep --pattern 'function $_($$_) {
$$$
/\.ts|\.js/
$$$
}'
Length of output: 1985
Script:
#!/bin/bash
# Search for file loader implementation that might handle extension precedence
rg -A 10 "\.ts|\.js" src/loader/file_loader.ts
# Look for any extension handling in the loader
rg -A 10 "extensions?" src/loader/file_loader.ts
# Check for any TypeScript-specific loading logic
rg -A 10 "typescript|\.ts" src/loader/file_loader.ts
Length of output: 4950
Script:
#!/bin/bash
# Check the utils extension configuration
rg -A 5 "extensions.*\.ts" src/utils/index.ts
Length of output: 47
Script:
#!/bin/bash
# Let's try a broader search for extensions configuration
rg -A 5 "extensions" src/utils/index.ts
# Also check if it's exported from another file
rg -l "export.*extensions" src/
# Look for any TypeScript-related configuration
rg -A 5 "typescript|EGG_TYPESCRIPT" src/utils/
Length of output: 615
| async render() { | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider providing an implementation for the render method.
Currently, the method body is empty, which may cause failing tests or unimplemented features if it's intended to mirror the JavaScript version that sets a 400 status and returns { foo: 'bar' }.
Do you want me to propose an implementation here and align it with the JavaScript counterpart?
[skip ci] ## [6.2.6](v6.2.5...v6.2.6) (2024-12-29) ### Bug Fixes * ignore js file when the same ts file exists ([#280](#280)) ([a6acc88](a6acc88))
Summary by CodeRabbit
New Features
Bug Fixes
.jsand.tsfilesChores
Tests