Skip to content

Add app router types #82439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Add app router types #82439

wants to merge 2 commits into from
+84 −3

Conversation

remcohaszing
Copy link
Contributor

What?

This change introduces some helper types for dealing with App router.

This also fixes the React import by changing it to a star import. The react module has no default export. Instead, Next.js relied on specific TypeScript configurations that may or may not work in some situations.

Why?

When dealing with app router, there were no types available. Instead, the user was supposed to define their own types. This is prone to typos and reduces discoverability of properties.

How?

By copying over some types I’ve already been happy to use in my own Next.js project.

@remcohaszing
Add app router types
f2af2f1 
When dealing with app router, there were no types available. Instead,
the user was supposed to define their own types. This is prone to typos
and reduces discoverability of properties.

This change introduces some helper types for dealing with App router.

This also fixes the `React` import by changing it to a star import. The
`react` module has no default export. Instead, Next.js relied on
specific TypeScript configurations that may or may not work in some
situations.
@ijjk ijjk added the type: next label Aug 7, 2025
@ijjk
Copy link
Member

ijjk commented Aug 7, 2025

Allow CI Workflow Run

  • approve CI run for commit: f2af2f1

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@ijjk
Copy link
Member

ijjk commented Aug 7, 2025 •
edited
Loading

Allow CI Workflow Run

  • approve CI run for commit: 262c327

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

graphite-app[bot]
graphite-app bot reviewed Aug 7, 2025
View reviewed changes
@remcohaszing @graphite-app
Update packages/next/src/types.ts
262c327 
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
graphite-app[bot]
graphite-app bot reviewed Aug 7, 2025
View reviewed changes
packages/next/src/types.ts
) => Metadata | PromiseLike<Metadata>
generateStaticParams?: () => Params[] | PromiseLike<Params[]>
metadata?: Metadata
default: React.ComponentType<PageProps<Params>>
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation for the default property is inconsistent with the other properties in the PageModule interface. This should be indented to the same level as generateMetadata, generateStaticParams, and metadata to properly define it as a property of the interface:

export interface PageModule<Params = {}> {
  generateMetadata?: (
    props: PageProps<Params>,
    parent: ResolvingMetadata
  ) => Metadata | PromiseLike<Metadata>
  generateStaticParams?: () => Params[] | PromiseLike<Params[]>
  metadata?: Metadata
  default: React.ComponentType<PageProps<Params>>
}
Suggested change
default: React.ComponentType<PageProps<Params>>
default: React.ComponentType<PageProps<Params>>

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

@austinderek austinderek mentioned this pull request Aug 7, 2025
Closed
@eps1lon eps1lon added the CI approved Approve running CI for fork label Aug 11, 2025
eps1lon
eps1lon requested changes Aug 11, 2025
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Thank you. Changing the React import makes sense but should be accompanied by a lint rule to prevent future regressions.

The helper types will be addressed by #81396

@ijjk
Copy link
Member

ijjk commented Aug 11, 2025

Failing test suites

Commit: 262c327

pnpm test-start test/e2e/app-dir/segment-cache/metadata/segment-cache-metadata.test.ts

  • segment cache (metadata) > regression: prefetch the head if it's missing even if all other data is cached > pages with runtime-prefetchable content and dynamic metadata, using a runtime prefetch
Expand output

● segment cache (metadata) › regression: prefetch the head if it's missing even if all other data is cached › pages with runtime-prefetchable content and dynamic metadata, using a runtime prefetch

thrown: "Exceeded timeout of 120000 ms for a test.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  75 |     })
  76 |
> 77 |     it('pages with runtime-prefetchable content and dynamic metadata, using a runtime prefetch', async () => {
     |     ^
  78 |       let act: ReturnType<typeof createRouterAct>
  79 |       const browser = await next.browser('/', {
  80 |         beforePageLoad(p) {

  at it (e2e/app-dir/segment-cache/metadata/segment-cache-metadata.test.ts:77:5)
  at describe (e2e/app-dir/segment-cache/metadata/segment-cache-metadata.test.ts:12:3)
  at Object.describe (e2e/app-dir/segment-cache/metadata/segment-cache-metadata.test.ts:4:1)

Read more about building and testing Next.js in contributing.md.

@remcohaszing
Copy link
Contributor Author

This React import is one of many imports. The correct way to check this is enabling the TypeScript option verbatimModuleSyntax. Given its impact, I doubt you’ll want to go all-in on that at once.

I look forward to using the alternative using generated types. I’ll close this PR :)

@remcohaszing remcohaszing deleted the app-router-types branch August 11, 2025 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@eps1lon eps1lon eps1lon requested changes

Assignees
No one assigned
Labels
CI approved Approve running CI for fork type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@remcohaszing @ijjk @eps1lon