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

14.1.1 version introducing breaking change for cookies() #62727

Closed
alimek opened this issue Mar 1, 2024 · 11 comments · Fixed by #62821
Closed

14.1.1 version introducing breaking change for cookies() #62727

alimek opened this issue Mar 1, 2024 · 11 comments · Fixed by #62821
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked

Comments

@alimek
Copy link

alimek commented Mar 1, 2024

Link to the code that reproduces this issue

https://github.com/alimek/next-bug

To Reproduce

export const getToken = cache(() => {
  const cookieStore = cookies();
  const token = cookieStore.get("token")?.value;

  return token ?? 'no-token';
});

export const getLanguage = cache(() => {
  const cookieStore = cookies();
  const language = cookieStore.get("language")?.value;

  return language ?? 'pl-pl';
});

when calling getLanguage() or getToken() it should return always string, after upgrade to 14.1.1 its Promise

Current vs. Expected behavior

Current

export default async function RootLayout({
 children,
}: Readonly<{
 children: React.ReactNode;
}>) {
const token = getToken();
const language = getLanguage();

 return (
   <html lang={language} suppressHydrationWarning>
     <body className={inter.className} suppressHydrationWarning>
       <Client value={token} />
     </body>
   </html>
 );
}

generating

<html lang="[object Promise]">
...
</html>

instead

<html lang="pl-pl">
...
</html>

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.3.0: Wed Dec 20 21:30:44 PST 2023; root:xnu-10002.81.5~7/RELEASE_ARM64_T6000
Binaries:
  Node: 18.19.0
  npm: 10.2.3
  Yarn: 4.1.0
  pnpm: 8.7.0
Relevant Packages:
  next: 14.1.1
  eslint-config-next: 14.1.0
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.3.3
Next.js Config:
  output: standalone

Which area(s) are affected? (Select all that apply)

Not sure

Which stage(s) are affected? (Select all that apply)

next dev (local), next build (local)

Additional context

No response

NEXT-2660

@alimek alimek added the bug Issue was opened via the bug report template. label Mar 1, 2024
@alimek alimek changed the title 14.1.1 version introducing breaking change for cookie() and header() 14.1.1 version introducing breaking change for cookies() Mar 1, 2024
@ztanner ztanner added the linear: next Confirmed issue that is tracked by the Next.js team. label Mar 1, 2024
@pookmish
Copy link

pookmish commented Mar 2, 2024

This appears very similar to an issue I found with 14.1.1. It's not cookies or headers, but I'm using graphql-request and with 14.1.0 I was able to just do something like await getMyGraphqlClient().Query(). With 14.1.1 I have to do await (await getMyGraphqlClient()).Query(). I wonder if something is unnecessarily wrapping functions in async?

I confirmed the issue occurs with 14.1.2-canary.1

@pookmish
Copy link

pookmish commented Mar 3, 2024

For my issue I discovered by having "use server"; at the top of the file, Next would convert all functions into async functions. As soon as I removed the "use server", it went back to expected behavior.

@feedthejim
Copy link
Contributor

hey thanks for reporting, we're hoping to fix the issue today or tomorrow! we will trigger a patch then

@shuding
Copy link
Member

shuding commented Mar 4, 2024

Hey everyone! It turned out to be a bug that previously it's allowed to mark synchronous functions with "use server", as explained in #62821. With that PR we're improving the messages so it can error earlier.

shuding added a commit that referenced this issue Mar 4, 2024
As mentioned in the new-added error messages, and the [linked
resources](https://react.dev/reference/react/use-server#:~:text=Because%20the%20underlying%20network%20calls%20are%20always%20asynchronous%2C%20%27use%20server%27%20can%20only%20be%20used%20on%20async%20functions.):

> Because the underlying network calls are always asynchronous, 'use
server' can only be used on async functions.
> https://react.dev/reference/react/use-server

It's a requirement that only async functions are allowed to be exported
and annotated with `'use server'`. Currently, we already have compiler
check so this will already error:

```js
'use server'

export function foo () {} // missing async
```

However, since exported values can be very dynamic the compiler can't
catch all mistakes like that. We also have a runtime check for all
exports in a `'use server'` function, but it only covers `typeof value
=== 'function'`.

This PR adds a stricter check for "use server" annotated values to also
make sure they're async functions (`value.constructor.name ===
'AsyncFunction'`).

That said, there are still cases like synchronously returning a promise
to make a function "async", but it's still very different by definition.
For example:

```js
const f = async () => { throw 1; return 1 }
const g = () => { throw 1; return Promise.resolve(1) }
```

Where `g()` can be synchronously caught (`try { g() } catch {}`) but
`f()` can't even if they have the same types. If we allow `g` to be a
Server Action, this behavior is no longer always true but depending on
where it's called (server or client).

Closes #62727.
shuding added a commit that referenced this issue Mar 4, 2024
As mentioned in the new-added error messages, and the [linked
resources](https://react.dev/reference/react/use-server#:~:text=Because%20the%20underlying%20network%20calls%20are%20always%20asynchronous%2C%20%27use%20server%27%20can%20only%20be%20used%20on%20async%20functions.):

> Because the underlying network calls are always asynchronous, 'use
server' can only be used on async functions.
> https://react.dev/reference/react/use-server

It's a requirement that only async functions are allowed to be exported
and annotated with `'use server'`. Currently, we already have compiler
check so this will already error:

```js
'use server'

export function foo () {} // missing async
```

However, since exported values can be very dynamic the compiler can't
catch all mistakes like that. We also have a runtime check for all
exports in a `'use server'` function, but it only covers `typeof value
=== 'function'`.

This PR adds a stricter check for "use server" annotated values to also
make sure they're async functions (`value.constructor.name ===
'AsyncFunction'`).

That said, there are still cases like synchronously returning a promise
to make a function "async", but it's still very different by definition.
For example:

```js
const f = async () => { throw 1; return 1 }
const g = () => { throw 1; return Promise.resolve(1) }
```

Where `g()` can be synchronously caught (`try { g() } catch {}`) but
`f()` can't even if they have the same types. If we allow `g` to be a
Server Action, this behavior is no longer always true but depending on
where it's called (server or client).

Closes #62727.
@alimek
Copy link
Author

alimek commented Mar 4, 2024

so, every function now which is in "use server" file must be asynchronous

For mine use case, I was just using "use server" to mark exported function to just force run it on the server, which I am not sure now if that is really the thing

I am now thinking if import "server-only" would be better for the use case? so I dont have to use await to get value from cookie as example?

@shuding
Copy link
Member

shuding commented Mar 4, 2024

Yes @alimek, import "server-only" is exactly for that use case. Even if you mark a file with "use server", client modules are still able to import and invoke these functions (Server Action calls). But if you use import "server-only" and try to import it into the client you'll directly get an error.

@alimek
Copy link
Author

alimek commented Mar 4, 2024

cool, thanks for clarification 👍

@ADTC
Copy link
Contributor

ADTC commented Mar 8, 2024

I have the below function which is used to get the cookie value, but now it gets a Promise. It broke my application.

'use server'

import { cookies } from 'next/headers'

// Definition:
export const getCookie = (name: string) => cookies().get(name)?.value

// Usage:
const myCookie = getCookie('myCookie')

How do I best fix this right now? I recently moved from Next version 14.1.0 to 14.1.3. I think I can temporarily treat it as a Promise and use await:

// Definition: no change (❌ see my next comment for updated solution)

// Usage:
const myCookie = await getCookie('myCookie')

This got it working again. TypeScript warns though: 'await' has no effect on the type of this expression. ts(80007)

The question is: Will it break in a future release? Non-Promise values should fall through await expressions without any error, but side effects regarding execution order can occur.

@alimek
Copy link
Author

alimek commented Mar 8, 2024

@ADTC assuming you had same problem like me, If you want to force your function be only available on server, change 'use server' to import 'server-only'; and it should work.

@ADTC
Copy link
Contributor

ADTC commented Mar 8, 2024

After a bit more reading, I seem to gather that all functions in a file that starts with 'use server' are now automatically async functions, even if they look synchronous.

And this is a permanent expectation of Server Actions that won't be reverted in a future release of Next.js. Is that right?

In that case, I infer that the best fix is to also change the definition to make it async. (Another more explicit way is to construct and return a new Promise.)

// Definition:
export const getCookie = async (name: string) => cookies().get(name)?.value

// Usage:
const myCookie = await getCookie('myCookie')

@alimek (You commented after I wrote the above already.) In my case, I cannot make your suggested change because I'm using these methods in client components as Server Actions. But yes, that's another possible solution if your methods are meant for server use only.

Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. 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 Mar 22, 2024
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. locked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants