Skip to content

fix(plugins): treat dynamic(import().then(m => m.X)) as re-export#263

Merged
BartWaardenburg merged 1 commit intofallow-rs:mainfrom
fmguerreiro:upstream/nextjs-dynamic
May 3, 2026
Merged

fix(plugins): treat dynamic(import().then(m => m.X)) as re-export#263
BartWaardenburg merged 1 commit intofallow-rs:mainfrom
fmguerreiro:upstream/nextjs-dynamic

Conversation

@fmguerreiro
Copy link
Copy Markdown
Contributor

Problem

The standard Next.js code-splitting idiom produces a false-positive duplicate-export:

```tsx
// Foo.tsx
export function Foo() { /* heavy component */ }

// Foo-lazy.tsx
export const Foo = dynamic(
() => import('./Foo').then(m => m.Foo),
{ ssr: false },
);
```

Both files appear in duplicate_exports for Foo, even though the lazy wrapper is semantically a re-export, equivalent to export { Foo } from './Foo' (which fallow already does not flag). Every Next.js codebase using next/dynamic to lazy-load Mapbox, charts, editors, or PDF viewers hits this. Documented pattern: https://nextjs.org/docs/app/api-reference/functions/dynamic

Fix

find_duplicate_exports accepts a &[ResolvedModule] and extends re_export_sources with dynamic-import edges that act as re-exports. The new helper collect_dynamic_reexport_sources (in crates/core/src/analyze/unused_exports.rs) walks resolved modules and, for each resolved_dynamic_imports entry targeting an InternalModule, adds the source as a re-export source of the wrapper if the wrapper also exports the same shape:

Imported name Wrapper must export Examples
Named("X") a Named export of X dynamic(() => import('./Foo').then(m => m.Foo))
Default a Default export dynamic(() => import('./Foo'))
Namespace/SideEffect never counts as a re-export namespace imports cannot re-export

The wrapper-must-export check is the load-bearing detail. Without it, any module with a dynamic import targeting another module would silently suppress all duplicate-export findings between them, even when the dynamic import is for some unrelated purpose.

Tests

5 unit tests in analyze::unused_exports::tests:

  • dynamic_import_then_member_not_flagged_as_duplicate (named via .then(m => m.X))
  • dynamic_import_without_then_default_not_flagged_as_duplicate (default-import variant; both modules have a default export)
  • dynamic_import_named_without_matching_export_still_flagged (wrapper has Named("Foo") import but exports "Bar"; duplicate must still be flagged)
  • dynamic_import_default_without_default_export_still_flagged (wrapper has Default import but only Named exports; duplicate must still be flagged)
  • duplicate_without_dynamic_link_still_flagged (regression guard for callers that pass non-empty resolved_modules without dynamic-import links)

The last three guard against false negatives.

API surface

find_duplicate_exports gains a new resolved_modules: &[ResolvedModule] parameter. The existing call site in analyze::find_dead_code_full passes the resolver output. Existing test call sites pass &[], preserving their semantics. Doc comment notes: pass &[] to opt out of dynamic-import detection; static export { X } from '...' re-exports continue to be recognized.

Verified on

cargo test --workspace, cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --all -- --check all clean. Real-world: this fork-pinned binary running in a Turborepo monorepo CI cleared the dynamic-wrapper false positive across multiple components.

The Next.js code-splitting idiom

    // Foo.tsx
    export function Foo() { /* heavy */ }

    // Foo-lazy.tsx
    export const Foo = dynamic(
      () => import('./Foo').then(m => m.Foo),
      { ssr: false },
    );

is semantically a re-export of `Foo`, equivalent to
`export { Foo } from './Foo'`, which fallow already does not flag.
Currently both files appear in `duplicate_exports` for `Foo`.

`find_duplicate_exports` now accepts the resolved module list and
extends `re_export_sources` with dynamic-import edges that act as
re-exports. The classification mirrors the static `export from` shape
exactly: a dynamic import counts as a re-export only when the wrapper
module also exports the same name (`Named("X")` requires the wrapper
to export "X"; `Default` requires the wrapper to have a default
export). `Namespace` and `SideEffect` imports never count as
re-exports.

The dynamic-reexport collection lives in a separate
`collect_dynamic_reexport_sources` helper to keep
`find_duplicate_exports` focused on duplicate detection.

Five unit tests:
- happy paths for `.then(m => m.Foo)` and default-import variants
- wrapper has Named dynamic import but exports a different name
- wrapper has Default dynamic import but no default export
- two unrelated modules with no dynamic link still flagged

The two missing-export tests guard against false negatives where a
module dynamically imports something but does not actually re-export
it.

All 16 existing call sites of `find_duplicate_exports` pass `&[]`
for the new parameter, preserving their semantics. Pass `&[]` to opt
out of dynamic-import re-export detection.
@BartWaardenburg BartWaardenburg merged commit 1438939 into fallow-rs:main May 3, 2026
18 checks passed
BartWaardenburg added a commit that referenced this pull request May 3, 2026
Three follow-ups surfaced during pre-ship review of #263 and #265:

1. #263 added five tests for the Next.js dynamic-import re-export fix.
   Four are regression-strength but `dynamic_import_named_without_matching_export_still_flagged`
   is coverage-adjacent: the wrapper exports `Bar` and the duplicate
   detection looks for `Foo` between two unrelated modules, so the
   wrapper's `matches_export` check is never on the path that decides
   the test outcome. Removing the Named branch of `matches_export`
   does not make this test fail. Add a true regression test
   (`dynamic_import_named_mismatched_with_wrapper_export_still_flagged`)
   where the wrapper exports `Foo` AND dynamically imports a different
   name `Bar` from source, so without the matches_export Named branch
   the wrapper would be registered as a re-export edge and the
   (source, wrapper) `Foo` duplicate would be silently suppressed.
   Empirically verified: removing the Named branch makes this test
   fail and leaves the other four passing.

2. #265 added `Plugin::virtual_package_suffixes()` but only via an
   explicit `impl Plugin for VitestPlugin` block. The three
   `define_plugin!` macro variants in `plugins/mod.rs` still listed
   `virtual_module_prefixes` without a peer entry for the new field,
   so a future plugin author using the macro would have no path to
   declare suffixes. Extend all three variants to accept
   `virtual_package_suffixes:` and add a synthetic smoke test to guard
   future macro regressions.

3. `.claude/rules/plugins.md` `## Plugin trait extensions` section
   listed `path_aliases` and `virtual_module_prefixes` but not the new
   `virtual_package_suffixes`. Add it with a one-line description and
   a Vitest example.
@BartWaardenburg
Copy link
Copy Markdown
Collaborator

Released in v2.63.0. Thanks @fmguerreiro.

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.

2 participants