fix: scan default class exports as default imports#544
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR fixes incorrect import generation for default class declarations. The ChangesDefault Class Declaration Export Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/scan-dirs.test.ts (1)
304-313: ⚡ Quick winTighten this regression test to assert exact output.
On Line 304-313, the test checks include/exclude conditions but still passes if extra exports are emitted. Assert exact equality to a single-item result to lock the behavior.
Suggested test tightening
- expect(exports).toContainEqual({ - name: 'default', - as: 'MyClass', - from: filepath, - }) - expect(exports).not.toContainEqual({ - name: 'MyClass', - as: 'MyClass', - from: filepath, - }) + expect(exports).toEqual([ + { + name: 'default', + as: 'MyClass', + from: filepath, + }, + ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/scan-dirs.test.ts` around lines 304 - 313, The test currently uses expect(exports).toContainEqual(...) and expect(...).not.toContainEqual(...) which allows extra entries; tighten it to assert exact output by replacing those two assertions with a single strict equality/assertion that exports equals exactly an array with one object { name: 'default', as: 'MyClass', from: filepath } (i.e., assert deep equality of the exports array to that single-item array) so no extra exports can be present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/scan-dirs.test.ts`:
- Around line 304-313: The test currently uses
expect(exports).toContainEqual(...) and expect(...).not.toContainEqual(...)
which allows extra entries; tighten it to assert exact output by replacing those
two assertions with a single strict equality/assertion that exports equals
exactly an array with one object { name: 'default', as: 'MyClass', from:
filepath } (i.e., assert deep equality of the exports array to that single-item
array) so no extra exports can be present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a4eeaa8-92e4-455f-8fbf-2a074ccc5376
📒 Files selected for processing (3)
src/node/scan-dirs.tstest/fixtures/default-class/MyClass.jstest/scan-dirs.test.ts
|
Since this is an issue caused by a bad upstream change (unjs/mlly#320), I'd recommend instead merging unjs/mlly#354 (which reverts that change) and, when ready, #542 (which re-fixes the issue the original bad change was intended to fix). I haven't looked into this too in-depth, so I could be wrong, but I assume this fix would be considered more of a workaround for the upstream problem rather than a sound solution to the underlying problem. |
|
Thanks, that makes sense. I will wait for maintainers to decide whether this local guard is still useful after the mlly fix path. |
2 participants
Fixes #541.
mlly.findExports()reports a named default class declaration twice: once as the real default export and once as a declaration named after the class.scanExports()then registered both imports, and the later dedupe step could keep the named import instead of the default import.This skips declaration entries that come from
export default ...soexport default class MyClass {}is scanned as:instead of:
Verification:
pnpm exec vitest run test/scan-dirs.test.tspnpm test -- --runpnpm lintpnpm typecheckpnpm buildgit diff --checkpnpm buildcompleted successfully with the existing Browserslist database warning.Summary by CodeRabbit
defaultimport is recorded and the class isn’t additionally registered as a named export.