Skip to content
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.

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

[NEXT-764] GET Route Handlers not valid destinations with experimental.typedRoutes #46742

Closed
1 task done
karlhorky opened this issue Mar 3, 2023 · 7 comments
Closed
1 task done
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. TypeScript Related to types with Next.js.

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Mar 3, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
      Platform: linux
      Arch: x64
      Version: #22 SMP Tue Jan 10 18:39:00 UTC 2023
    Binaries:
      Node: 16.17.0
      npm: 8.15.0
      Yarn: 1.22.19
      pnpm: 7.1.0
    Relevant packages:
      next: 13.2.4-canary.1
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0

Which area(s) of Next.js are affected? (leave empty if unsure)

App directory (appDir: true), Middleware / Edge (API routes, runtime), Routing (next/router, next/navigation, next/link), TypeScript

Link to the code that reproduces this issue

https://codesandbox.io/p/sandbox/trusting-rosalind-1mwkuc

To Reproduce

  1. Clone CodeSandbox and enable local TypeScript version + Next.js TypeScript plugin
  2. Observe the TypeScript error in app/page.tsx about the href location pointing at a GET Route Handler
Type '"/set-cookie"' is not assignable to type 'UrlObject | RouteImpl<"/set-cookie">'.ts(2322)

link.d.ts(76, 5): The expected type comes from property 'href' which is declared here on type 'IntrinsicAttributes & LinkRestProps & HrefProp<"/set-cookie">'

Associated Route Handler:

app/set-cookie/route.ts

import { NextRequest, NextResponse } from "next/server";

export async function GET(request: NextRequest) {
  const maxAge = 60 * 60 * 24; // 24 hours in seconds
  return new NextResponse(null, {
    // Temporary redirect
    status: 307,
    headers: {
      location: "/",
      "Set-Cookie": `cookie_name=a3fWa; Max-Age=${maxAge}; Expires: ${
        Date.now() + maxAge * 1000
      }; Path=/`,
    },
  });
}

Describe the Bug

The experimental.typedRoutes configuration doesn't handle an href prop on next/link pointing at a location of a GET Route Handler

cc @DuCanhGH @shuding

Expected Behavior

An href location pointing at a GET Route Handler works

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

From SyncLinear.com | NEXT-764

@karlhorky karlhorky added the bug Issue was opened via the bug report template. label Mar 3, 2023
@github-actions github-actions bot added the TypeScript Related to types with Next.js. label Mar 3, 2023
@shuding shuding added the linear: next Confirmed issue that is tracked by the Next.js team. label Mar 3, 2023
@shuding shuding changed the title GET Route Handlers not valid destinations with experimental.typedRoutes [NEXT-764] GET Route Handlers not valid destinations with experimental.typedRoutes Mar 3, 2023
@DuCanhGH
Copy link
Contributor

DuCanhGH commented Mar 3, 2023

image
We currently filter out non-page files from type Route. I think I will also add support for route.ts in my current PR.

@karlhorky
Copy link
Contributor Author

karlhorky commented Mar 3, 2023

I think I will also add support for route.ts in my current PR.

This PR, I imagine?

Maybe a line should be added to the top of the PR description with Fixes #46742 ? This will cause GitHub to automatically close this issue when your PR is merged.

@DuCanhGH
Copy link
Contributor

DuCanhGH commented Mar 3, 2023

@karlhorky I'm asking for the members' reviews first, since I don't know whether Route Handlers are supposed to be used with next/link or not. It just happens that this issue is tagged with [NEXT-*], so I thought why not, yeah.

@shuding
Copy link
Member

shuding commented Mar 4, 2023

@DuCanhGH Let's open a separate PR for this feature request as this one requires more complicated logic to implement and needs some discussion (typed routes doesn't support API routes and public files at all as of today, not a bug).

@shuding
Copy link
Member

shuding commented Mar 15, 2023

I shared some of my thoughts here: #47185 (comment), but this does look like a valid use case... although as Route is a work around here, do you feel that we should add GET routes to the types cc @sebmarkbage?

@sebmarkbage
Copy link
Contributor

Yea, ideally you should be able to use a custom route with a rewrite just as if it was a Next.js built-in page. So we should include those.

@shuding shuding closed this as completed Apr 4, 2023
karlhorky added a commit to upleveled/eslint-config-upleveled that referenced this issue Apr 5, 2023
Now that Next.js has support for linking to GET Route Handlers, we no longer
need this patch

Ref: vercel/next.js#46742
Ref: vercel/next.js#47185
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. TypeScript Related to types with Next.js.
Projects
None yet
Development

No branches or pull requests

4 participants