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

feat: add Next.js 15+ support #86

Merged
merged 18 commits into from
Nov 8, 2024
Merged

Conversation

joaopcm
Copy link
Contributor

@joaopcm joaopcm commented Oct 22, 2024

If you try to use cookies-next with the brand-new Next.js version 15.0.0, your app will break due to the new async usage of cookies() from next/headers.

This PR introduces a new breaking change for cookies-next, version 5.0.0, which adds support for the new Next.js version as well, so we can continue using it in our projects while we upgrade them to the latest Next.js version.

Additionally, I took the opportunity to add a certain level of tests to ensure it works as expected and updated its documentation to clarify and reflect the new changes from the Next.js team.

I have a big project running on top of Next.js 14. Yesterday, I tried to upgrade it to Next.js 15, but it started to fail due to the incompatibility between the new Next.js next/headers API and cookies-next, so I decided to bring up this PR.

After finishing the changes for this library, I tested it in that project already migrated to the latest Next.js version and it just works.

I hope you find this helpful! Happy to help! Cheers!

@joaopcm
Copy link
Contributor Author

joaopcm commented Oct 22, 2024

@andreizanik I'd love to hear from you if it makes sense :) Thanks!

@greg2012201
Copy link
Contributor

greg2012201 commented Oct 22, 2024

Hello @joaopcm!

Great job! Thanks to you, we’re almost ready to support Next.js v15 just one day after its release. I noticed one key point in your PR that I believe needs adjustment. From what I understand, your changes make every function asynchronous, which is fine overall. However, for client components, we should keep the synchronous versions. Otherwise, users will need to add extra code in client components to handle this behavior.

I suggest that we return promises only for the server-side code. I realize this will require some extra if statements, particularly for functions like setCookie, hasCookie, and deleteCookie, but I think it’s worth the effort. What are your thoughts?

@joaopcm
Copy link
Contributor Author

joaopcm commented Oct 22, 2024

Hey @greg2012201!

It makes sense! I'll work on it and update the PR. Can I mention you once I get it done so we can review it again?

@greg2012201
Copy link
Contributor

greg2012201 commented Oct 22, 2024

Sure! I'm looking foward to your updates.

package.json Outdated Show resolved Hide resolved
src/cookies.test.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@joaopcm joaopcm mentioned this pull request Oct 23, 2024
@dczii dczii mentioned this pull request Oct 23, 2024
@will-stone
Copy link

Does this change the peerDependency of this package from Next 14 to Next 15 or will it support both? Mind adding that entry into package.json so consumers are aware which version(s) it supports?

Great work chaps! Thank you.

-  replaced  and  with  and  for better clarity and alignment with Next.js conventions
-  simplified cookie transformation and validation functions to use
-  removed redundant type definitions and moved relevant types to a centralized file
-  improved error handling by formatting error messages for better readability
-  deleted unused  file to reduce code duplication and maintain consistency across the codebase
@joaopcm
Copy link
Contributor Author

joaopcm commented Oct 23, 2024

Hey @will-stone @greg2012201 @andreizanik,

I thought a lot about how to keep the DX for client-side environments synchronous and maintain asynchronicity only for server-side environments.

I made a small adjustment to the library's structure. The current (or old) way of using it remains valid. You can import functions directly from cookies-next. However, if you prefer, you can import directly from cookies-next/client for client-side components or cookies-next/server to use the asynchronous API.

If the import is done directly from cookies-next, the library will conditionally handle identifying which environment you are using. If it's client-side, the exported methods will be synchronous. Otherwise, the asynchronous server-side exclusive methods will be imported.

If someone tries to import /client and use it on the server-side, we will throw an exception. The opposite also happens; if you try to use server-side-only methods in a client-side environment, there will be an exception because the internal APIs are different.

That said, here are some other changes I made:

  • Peer Dependency: Version 5.0.0 of cookies-next now requires at least Next.js 15.0.0 (cc @will-stone);
  • Documentation: I updated the documentation to include the new differentiation between client-side and server-side. I also kept all the examples that @andreizanik asked us to keep previously;
  • Tests: I added tests for various cases to ensure the tool works as expected. I did not include a GitHub Action, but this can be improved in a future PR; and
  • cookie Version Bumped: I updated the version of cookie, used internally.

Given all this context, the PR is ready to be reviewed again @greg2012201 @andreizanik.

I also want to clarify it that it is not enough for the tests with Jest to pass for both client and server side, but I also ensured to test the full functionality of version 5.0.0 in an existing project with Next.js 15.0.0 on both server and client side.

Server-side tests passing

CleanShot 2024-10-23 at 7  04 24@2x

Client-side tests passing

CleanShot 2024-10-23 at 7  04 47@2x

Hit me up if you need anything from me! Happy to help! Thank you!

@joaopcm joaopcm requested a review from greg2012201 October 23, 2024 22:08
@joaopcm joaopcm requested a review from andreizanik October 23, 2024 22:08
@andreizanik
Copy link
Owner

Hey @joaopcm
Excellent job! Thanks

I need to look at everything in more detail, but I'll take a while because because I'm mad busy with work

@greg2012201
Copy link
Contributor

greg2012201 commented Oct 24, 2024

Nice approach @joaopcm! I appreciate the code structure reorganization and have delved into your changes. I tested them on both the client and server components. However, there's an issue in the client component during its server phase, as it relies on window. When the client component is pre-rendered on the server, getCookie runs, but an exception is thrown:
throw new Error('You are trying to access cookies on the server side. Please, use the server-side import with cookies-next/server instead.'). This occurs when importing cookies-next/client. Additionally, when using the cookies-next import, the getCookie function returns an unresolved promise(client component - server phase).

I believe we should avoid relying on window and instead determine the environment based on the options passed. If server-specific options like cookies or req are provided, we can infer it's the server phase; otherwise, it’s the client.

PS: I haven't tested it in middleware, server actions, route handlers

@APeltick
Copy link

Hi @joaopcm, your check ensureClientSide unfortunately doesn't cover case when you use cookie inside client component without wrap in hooks, so in render time window is undefined and I get error.

@APeltick
Copy link

Hi @joaopcm, your check ensureClientSide unfortunately doesn't cover case when you use cookie inside client component without wrap in hooks, so in render time window is undefined and I get error.

So just changing ensureClientSide(); to isClientSide() fix error and my app works as was before, btw @joaopcm thx for implemetation

@joaopcm
Copy link
Contributor Author

joaopcm commented Oct 25, 2024

Guys, I'll be a bit busy in the upcoming days and might not have enough time to keep this PR moving forward until the end of next week.

If you want to take this PR over, feel free to do it! Otherwise, I'll get back and update it in a few days. Sorry for that!

@greg2012201
Copy link
Contributor

greg2012201 commented Oct 29, 2024

@joaopcm @andreizanik This is the continuation: #87

@andreizanik andreizanik merged commit 455f774 into andreizanik:master Nov 8, 2024
@bombillazo
Copy link

Hey, we tried to upgrade the package but now we're getting Type errors since the serve cookie functions return promises.

Is there any way to use the updated lib without the use of promises for server?

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.

6 participants