Skip to content

fix: remove duplicate next-shims.d.ts from fixtures#1170

Merged
james-elicx merged 4 commits into
cloudflare:mainfrom
Deepam02:fix/consolidate-fixture-shims
May 12, 2026
Merged

fix: remove duplicate next-shims.d.ts from fixtures#1170
james-elicx merged 4 commits into
cloudflare:mainfrom
Deepam02:fix/consolidate-fixture-shims

Conversation

@Deepam02
Copy link
Copy Markdown
Contributor

fixes #1169

three test fixtures had their own copy of next-shims.d.ts duplicating what already exists in the main package. Removed those and pointed each fixture's tsconfig at the single shared file instead.

Had to add a few missing declarations to the shared file first before removing the copies.

Signed-off-by: Deepam Goyal <deepam02goyal@gmail.com>
@james-elicx
Copy link
Copy Markdown
Member

/bigbonk review

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 11, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1170

commit: f9df3b7

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

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

Good cleanup — consolidating the duplicated .d.ts files is the right call. Two issues need fixing before merge.

CI is red

The Check job fails with 4 consistent-type-definitions lint errors. The newly added declarations use interface where this repo's lint config requires type. The fix is mechanical — see inline suggestions below.

Scope observation

The examples/ directory still has two stale next-shims.d.ts copies (realworld-api-rest, pages-router-cloudflare) that are also subsets of the shared file. Those are out of scope for this PR (fixtures ≠ examples), but worth noting for a follow-up.

Comment thread packages/vinext/src/shims/next-shims.d.ts Outdated
Comment thread packages/vinext/src/shims/next-shims.d.ts Outdated
Comment thread packages/vinext/src/shims/next-shims.d.ts Outdated
Comment thread packages/vinext/src/shims/next-shims.d.ts Outdated
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 11, 2026

Review submitted. Here's a summary of the feedback:

Request changes on PR #1170 with two main points:

  1. CI is failing — The Check job has 4 consistent-type-definitions lint errors. The newly added declarations in next-shims.d.ts use interface but the repo's lint config requires type. I left inline suggestions with the exact fixes for all 4 locations (lines 11, 16, 110, and 507).

  2. Scope note — The examples/ directory still has two stale next-shims.d.ts copies (realworld-api-rest and pages-router-cloudflare) that are also subsets of the shared file. Noted as a follow-up opportunity, not a blocker for this PR.

github run

Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>
@Deepam02
Copy link
Copy Markdown
Contributor Author

@james-elicx Should I clean up the examples copies too or is that out of scope?

@james-elicx
Copy link
Copy Markdown
Member

That would be great!

@james-elicx
Copy link
Copy Markdown
Member

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

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

Lint fixes look good — the type intersections from the first review are all addressed, and CI (vp check) passes clean. Tests all pass too (313 app-router, 204 pages-router, 916 shims+link).

One real bug to fix before merge, and a minor note.

Bug: next/link default export references undeclared value

The declare module "next/link" block defines LinkProps as a type but never declares Link as a value — then does export default Link referencing thin air. The deleted fixture copies all had const Link: ComponentType<LinkProps>; before the export. This is masked by skipLibCheck: true everywhere, but the declaration is incorrect — consumers get any for the default import.

Follow-up: examples cleanup

Per the thread with @james-elicx, the two stale copies in examples/ (realworld-api-rest, pages-router-cloudflare) should be cleaned up in this PR as well.

onNavigate?: (event: { preventDefault(): void }) => void;
children?: ReactNode;
} & Omit<AnchorHTMLAttributes<HTMLAnchorElement>, "href">;
export default Link;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: Link is referenced here but never declared as a value. The deleted fixture copies all had a const Link declaration. Without it, the default export resolves to any (hidden by skipLibCheck).

Suggested change
export default Link;
const Link: ComponentType<LinkProps>;
export default Link;

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 12, 2026

Review submitted. Here's a summary:

One bug found, one follow-up noted.

  1. Bug — next/link missing value declaration (next-shims.d.ts:121): The declare module "next/link" block does export default Link but never declares Link as a value. The deleted fixture copies all had const Link: ComponentType<LinkProps>; before the export. Without it, the default import resolves to any. This is masked by skipLibCheck: true but the declaration is incorrect. Left an inline suggestion with the fix.

  2. Follow-up — examples cleanup: Per the existing thread, the two stale next-shims.d.ts copies in examples/realworld-api-rest and examples/pages-router-cloudflare should also be consolidated in this PR.

Everything else looks good — lint fixes from the first review are all addressed, vp check passes clean, and all tests pass (313 app-router, 204 pages-router, 916 shims+link).

github run

Apply the same consolidation from fixtures to examples/realworld-api-rest
and examples/pages-router-cloudflare. Point their tsconfigs at the shared
packages/vinext/src/shims/next-shims.d.ts instead of keeping local copies.
The deleted fixture copies all had `const Link: ComponentType<LinkProps>`
before the default export. Without it the default import resolves to `any`
(masked by skipLibCheck). Add the missing value declaration.
@james-elicx
Copy link
Copy Markdown
Member

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

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

All feedback from prior reviews has been addressed:

  1. Lint fixes — All four consistent-type-definitions violations (interfacetype intersection) are fixed. vp check passes.
  2. next/link value declarationconst Link: ComponentType<LinkProps> is present before export default Link (line 121). Bug fixed.
  3. Examples cleanup — Both examples/realworld-api-rest/next-shims.d.ts and examples/pages-router-cloudflare/next-shims.d.ts are removed; their tsconfigs point to the shared file.

Verified:

  • All 14 module declarations from the deleted fixtures/examples exist in the shared file (it's a superset).
  • All relative paths in tsconfig include arrays resolve correctly (../../../ for fixtures, ../../ for examples).
  • No stale next-shims.d.ts copies remain anywhere in the repo.
  • Check CI is green.

Clean consolidation — LGTM.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 12, 2026

Review submitted. Here's the summary:

Approved PR #1170. All feedback from the two prior review rounds has been addressed:

  1. Lint fixes — All four consistent-type-definitions violations are fixed (interfaces converted to type intersections).
  2. next/link bug — The missing const Link value declaration is now present before export default Link.
  3. Examples cleanup — Both stale copies in examples/ are removed and their tsconfigs updated.

Verified that all 14 module declarations from deleted files exist in the shared file, all relative paths resolve correctly, and no stale copies remain anywhere in the repo. Check CI is green; remaining Vitest and E2E jobs are still pending.

github run

@james-elicx james-elicx enabled auto-merge (squash) May 12, 2026 11:22
@james-elicx james-elicx merged commit 97f878a into cloudflare:main May 12, 2026
24 checks passed
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.

Remove need for shims type declarations in fixtures

2 participants