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

Always execute the <Metadata> component on the client #10271

Merged
merged 3 commits into from
May 29, 2024

Conversation

cannikin
Copy link
Member

@cannikin cannikin commented Mar 21, 2024

Right now in an RSC app if you use <Metadata> in a page or other component rendered on the server, the serve process dies with the error:

12:34:13 PM [vite] Error when evaluating SSR module /@fs/Users/rob/Sites/rsc/cambium-rsc/web/dist/rsc/assets/index-DqR_YzqW.mjs:
|- TypeError: React3.createContext is not a function
    at eval (/Users/rob/Sites/rsc/cambium-rsc/web/dist/rsc/assets/index-DqR_YzqW.mjs:42731:22)
    at async instantiateModule (file:///Users/rob/Sites/rsc/cambium-rsc/node_modules/vite/dist/node/chunks/dep-jvB8WLp9.js:54897:9)

node:events:496
      throw er; // Unhandled 'error' event
      ^

TypeError: React3.createContext is not a function
    at eval (/Users/rob/Sites/rsc/cambium-rsc/web/dist/rsc/assets/index-DqR_YzqW.mjs:42731:22)
    at async instantiateModule (file:///Users/rob/Sites/rsc/cambium-rsc/node_modules/vite/dist/node/chunks/dep-jvB8WLp9.js:54897:9)
Emitted 'error' event on PassThrough instance at:
    at emitErrorNT (node:internal/streams/destroy:169:8)
    at emitErrorCloseNT (node:internal/streams/destroy:128:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v20.11.1
------------------------------------------------------------------------------------------------------------------------
Error: Command failed with exit code 1: yarn rw-serve-fe
    at makeError (/Users/rob/Sites/rsc/cambium-rsc/node_modules/execa/lib/error.js:60:11)
    at handlePromise (/Users/rob/Sites/rsc/cambium-rsc/node_modules/execa/index.js:118:26)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Promise.all (index 1)
    at async bothSsrRscServerHandler (/Users/rob/Sites/rsc/cambium-rsc/node_modules/@redwoodjs/cli/dist/commands/serveBothHandler.js:76:3)
    at async Object.handler (/Users/rob/Sites/rsc/cambium-rsc/node_modules/@redwoodjs/cli/dist/commands/serve.js:47:9)
    at async runYargs (/Users/rob/Sites/rsc/cambium-rsc/node_modules/@redwoodjs/cli/dist/index.js:191:3)
    at async /Users/rob/Sites/rsc/cambium-rsc/node_modules/@redwoodjs/cli/dist/index.js:137:7
    at async main (/Users/rob/Sites/rsc/cambium-rsc/node_modules/@redwoodjs/cli/dist/index.js:115:3)

Tobbe suggested that something in <Metadata> or possibly Helmet is doing createContext internally, an so all of this code can only run on the client. The solution is to simply add 'use client' to the top of the Metadata.ts file.

Otherwise throws a TypeError when serving the site, `React.createContext is not a function`
@cannikin cannikin added the bug/confirmed We have confirmed this is a bug label Mar 21, 2024
@cannikin cannikin added this to the RSC milestone Mar 21, 2024
@cannikin cannikin self-assigned this Mar 21, 2024
@cannikin cannikin changed the title Adds 'use client' to Metadata so that it always runs in the client Always execute the <Metadata> component on the client Mar 21, 2024
@cannikin cannikin added fixture-ok Override the test project fixture check release:chore This PR is a chore (means nothing for users) labels Mar 21, 2024
@Tobbe
Copy link
Member

Tobbe commented Apr 1, 2024

Actually trying this code I get an error like

Error [RollupError]: src/pages/AboutPage/AboutPage.tsx (6:9): "Metadata" is not exported by "../node_modules/@redwoodjs/web/dist/components/Metadata.js", imported by "src/pages/AboutPage/AboutPage.tsx".
file: /Users/tobbe/tmp/rw-rsc-metadata/web/src/pages/AboutPage/AboutPage.tsx:6:9

It's interesting that it says that it's not exported, because looking at the file I clearly see this

image

I suspect, but have not been able to verify yet, that this is a CJS vs ESM issue

@cannikin
Copy link
Member Author

cannikin commented Apr 1, 2024

Yep I saw that same thing after adding 'use client' and then importing directly from dist.

@Tobbe
Copy link
Member

Tobbe commented Apr 1, 2024

Looking into this a bit more, it actually does look like it's us that are using context. Metadata imports PortalHead which imports ServerInject, and ServerInject uses context. So that's probably where we should add use client.

Or, maybe the real solution is to have two versions of Metadata, one that's to be used in server components and one for client components. But that might be getting ahead of ourselves a little bit 😅

@Tobbe Tobbe marked this pull request as ready for review May 29, 2024 21:21
@Tobbe
Copy link
Member

Tobbe commented May 29, 2024

With all the other changes that have happened since this PR was opened this now works!

And putting 'use client' in Metadata.tsx is exactly the right place. It imports react-helmet-sync which internally uses context. And it's not specifying 'use client'. So we have to.

@Tobbe Tobbe merged commit 06c1c8c into main May 29, 2024
46 checks passed
@Tobbe Tobbe deleted the rc-metadata-use-client branch May 29, 2024 21:42
@cannikin
Copy link
Member Author

If React 19 can move <meta> tags into the head automatically, maybe we don't need to use react-helmet-sync anymore?

dac09 added a commit to dac09/redwood that referenced this pull request May 30, 2024
…into feat/location-serverStore

* 'feat/location-serverStore' of github.com:dac09/redwood:
  no need to repeat fullUrl
  Use Replay for smoke tests (redwoodjs#10664)
  chore(testing): Remove unused member in Props interface (redwoodjs#10699)
  fix(cli): Directive generator command was not creating files (redwoodjs#10698)
  Always execute the <Metadata> component on the client (redwoodjs#10271)
@Josh-Walker-GM Josh-Walker-GM modified the milestones: RSC, v8.0.0 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug fixture-ok Override the test project fixture check release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants