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

NextJs options added to fetch #1571

Closed

Conversation

petercsaki
Copy link

Changes

NextJs uses fetch's options to pass information about caching. These were removed by openapi-fetch.

#1569 (comment)

Checklist

  • Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

Copy link

changeset-bot bot commented Feb 28, 2024

🦋 Changeset detected

Latest commit: 7191859

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openapi-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@drwpow
Copy link
Contributor

drwpow commented Feb 28, 2024

Thank you for opening! I’m +1 in favor of fixing this issue, and the test is perfect! But I would like to solve this without having any Next.js-specific code in the runtime. This probably means somehow sniffing out RequestInit and passing it along before/after each middleware (maybe we make new Response() from the URL + init each time? Unsure, but there’s probably a creative way to accomplish this).

If we can make that test pass in a generic way without looking for .next, then we’ll not only solve for Next.js; we’ll solve for all other libraries that are experiencing the same issue.

Copy link
Contributor

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

+1 in favor of this merging, and this is a great fix! But as we discussed I’d like to solve this without manually declaring .next. Perhaps there’s a more universal way we can solve this (such as manually rebuilding new Request()s, or another creative solution)?

If we had to have framework-specific code in openapi-fetch, we’d always be playing catchup. There’d always be a new vendor prefix (or existing ones could change) and there’d always be something left out and another version to ship. But solving for any arbitrary property added to the request would let us solve this problem once and for all.

@petercsaki
Copy link
Author

Yep, I get it. I'll need to think about how to get the parameters that aren't used in creating the Request object and should be passed as the second parameter for fetch.

@JE-lee
Copy link
Contributor

JE-lee commented Mar 6, 2024

I have checked the document of the fetch in Next.js and MDN, as well as the codebase of openapi-fetch. After reviewing them, I believe that there may be no need to add an external option to achieve the desired result. Instead, we can simply add the Next.js options as follows:

  const res = await client.GET("/api", { next: {revalidate: 10}})

I haven't tested it yet. Have you tried this @petercsaki

@petercsaki
Copy link
Author

petercsaki commented Mar 6, 2024

Yes. Openapi-fetch uses them as parameters when creating the Request object:

https://github.com/drwpow/openapi-typescript/blob/9c277fb0a10c3513de46765a4381ccb722a72af4/packages/openapi-fetch/src/index.js#L72C5-L75C7

but the request object only uses the properties of it that it knows about and others are "forgotten". The code uses only the Request object from then on. So any custom properties are ignored.

@JE-lee
Copy link
Contributor

JE-lee commented Mar 6, 2024

Yes. Openapi-fetch uses them as parameters when creating the Request object:

https://github.com/drwpow/openapi-typescript/blob/9c277fb0a10c3513de46765a4381ccb722a72af4/packages/openapi-fetch/src/index.js#L72C5-L75C7

but the request object only uses the properties of it that it knows about and others are "forgotten". The code uses only the Request object from then on. So any custom properties are ignored.

Haha, I got what you mean. I have tested some cases. It seems like The Next.js extended Fetch api only takes the second parameter as the cache option.
The following code works as well as the cache mechanism.

  const request = new Request('http://localhost:3000/api/v1/space', {
    method: 'GET',
  })
  const res = await fetch(request, { next: { revalidate: 10 } }) 

so you are in the right way. keep going.

@IngoVals
Copy link

IngoVals commented Mar 8, 2024

Would it be possible to switch NextJsFetchOptions out for AdditionalProps which is defined as an empty object that we spread into the fetch. Then in each case users could ts declare AdditionalProps to what they want ( nextjs props for example ).

declare module 'openapi-fetch' {
    type AdditionalProps {
        next?: { revalidate?: false | 0 | number; tags?: string[] };
     }
}

Just brainstorming.

@drwpow
Copy link
Contributor

drwpow commented Mar 8, 2024

@IngoVals love that suggestion! That would be a great addition, and I see the value in typechecking additional properties like that. That would be much better than just adding [key: string]: unknown to the shape.

That could either be part of this PR at the author’s discretion, or as a followup.

@ryami333
Copy link

ryami333 commented Mar 19, 2024

Is this even necessary? NextJS uses "ambient modules" to mutate RequestInit, which openapi-typescript already extends for FetchOptions, so I figure this should already work (as long as you've generated Next types at least once, with next lint).

@FreeAoi
Copy link
Contributor

FreeAoi commented Apr 28, 2024

How's going this PR?

@supercute
Copy link

I very wait this PR too ;)

@petercsaki
Copy link
Author

I've abandoned this, sorry. Someone pointed out that you can pass a custom fetch to each request, and that can use next's cahcing. That solution works for us. Creating a generic solution is not in my scope. Feel free to pick this up.

@drwpow
Copy link
Contributor

drwpow commented May 2, 2024

No worries! I’m going to close this then, but someone else is free to pick this up, provided my restriction up above: the solution should allow for any arbitrary properties, not just .next

@drwpow drwpow closed this May 2, 2024
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.

None yet

7 participants