Skip to content

fix(toTypeReExports): dedupe duplicate type re-exports for class/enum declarations#548

Open
greymoth-jp wants to merge 1 commit into
unjs:mainfrom
greymoth-jp:fix/type-reexports-dedupe
Open

fix(toTypeReExports): dedupe duplicate type re-exports for class/enum declarations#548
greymoth-jp wants to merge 1 commit into
unjs:mainfrom
greymoth-jp:fix/type-reexports-dedupe

Conversation

@greymoth-jp

@greymoth-jp greymoth-jp commented Jun 29, 2026

Copy link
Copy Markdown

toTypeReExports maps every type import in a module to an export type { ... } specifier without collapsing duplicates. dedupeImports keeps class, enum and const enum imports un-collapsed so a value and a type entry of the same declaration can coexist, which means the same type name can reach this step more than once. When it does, the generated re-export lists it twice:

import { createUnimport } from 'unimport'

const { generateTypeDeclarations } = createUnimport({
  imports: [
    { name: 'Foo', from: 'module.js', type: true, declarationType: 'class' },
    { name: 'Foo', from: 'module.js', type: true, declarationType: 'class' },
  ],
})

await generateTypeDeclarations()
// // for type re-export
// declare global {
//   // @ts-ignore
//   export type { Foo, Foo } from 'module'
//   import('module')
// }

export type { Foo, Foo } is a duplicate identifier (TS2300). It is currently suppressed because the line sits under the existing // @ts-ignore, but the output is still malformed. toExports got the same dedupe in #529; this brings the type re-export in line.

The change collapses entries by their re-exported name (as ?? name) before joining, so distinct names and aliases are untouched. The added test reproduces the duplicate: reverting only the source change makes it fail with export type { Foo, Foo }, and the full suite stays green.

Summary by CodeRabbit

  • Bug Fixes
    • Type re-exports now avoid duplicate entries when multiple imports refer to the same exported name.
    • Generated export statements are cleaner and no longer repeat the same type specifier.
  • Tests
    • Added coverage to verify duplicate type re-exports are collapsed into a single entry.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

toTypeReExports in src/utils.ts is updated to deduplicate type export specifiers using a Set of seen exported names, skipping repeats. A new test in test/dts.test.ts validates that duplicate Foo class/enum type imports produce a single export type { Foo } entry.

Type Re-export Deduplication

Layer / File(s) Summary
De-duplication logic and test
src/utils.ts, test/dts.test.ts
toTypeReExports now uses a seen Set to skip duplicate exported names when building typeImportNames; new snapshot test asserts one export type { Foo } for two identical class-typed imports.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Possibly related PRs

  • unjs/unimport#529: Applies the same deduplication pattern to toExports for value/type entries, directly complementing this PR's fix to toTypeReExports.

Suggested reviewers

  • antfu

🐇 Two Foos walked into a re-export,
The Set said "one's enough, I'll report!"
Now the types stay neat,
No duplicate feat—
Just one tidy specifier to escort. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: deduplicating type re-exports in toTypeReExports.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/dts.test.ts (1)

150-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add enum and const-enum cases to this regression.

This pins the class path, but the stated contract also covers enum and const enum. Extending this into a small table-driven test would keep those two variants from drifting untested.

🤖 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/dts.test.ts` around lines 150 - 183, The regression test currently
covers only the class re-export path in generateTypeDeclarations, but the dedupe
behavior is also expected for enum and const enum declarations. Update the
existing test in test/dts.test.ts (the createUnimport / generateTypeDeclarations
case) to include enum and const enum variants, preferably in a small
table-driven form, so all three declarationType values are asserted to produce a
single re-export and avoid future drift.
🤖 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/dts.test.ts`:
- Around line 150-183: The regression test currently covers only the class
re-export path in generateTypeDeclarations, but the dedupe behavior is also
expected for enum and const enum declarations. Update the existing test in
test/dts.test.ts (the createUnimport / generateTypeDeclarations case) to include
enum and const enum variants, preferably in a small table-driven form, so all
three declarationType values are asserted to produce a single re-export and
avoid future drift.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2ec4f468-1880-451e-9203-777ee11d5b5a

📥 Commits

Reviewing files that changed from the base of the PR and between e3c4752 and 546f7f6.

📒 Files selected for processing (2)
  • src/utils.ts
  • test/dts.test.ts

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.

1 participant