transitive-bullshit commented 1 year ago

Opening an issue to track support for Next.js 13's appDir and server components.

Currently, it's possible to use next-themes within appDir with the following pattern:

// app/layout.tsx
import * as React from 'react'

import { Providers } from './providers'
import './globals.css'

export default function RootLayout({
  children
}: {
  children: React.ReactNode
}) {
  return (
    <html>
      <head>
      </head>

      <body>
        <Providers>
          {children}
        </Providers>
      </body>
    </html>
  )
}

// app/providers.tsx
'use client'

import { ThemeProvider } from 'next-themes'

export function Providers({ children }: { children: React.ReactNode }) {
  return (
    <ThemeProvider>
      {children}
    </ThemeProvider>
  )
}

This works for the most part, including accessing and changing the theme via useTheme.

However, during next dev, we get the following console error hinting at hydration problems:

Warning: Extra attributes from the server: data-theme,style
    at html
    at ReactDevOverlay (webpack-internal:///./node_modules/.pnpm/next@13.0.3_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/client/components/react-dev-overlay/internal/ReactDevOverlay.js:53:9)
    at HotReload (webpack-internal:///./node_modules/.pnpm/next@13.0.3_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/client/components/react-dev-overlay/hot-reloader-client.js:19:11)
    at Router (webpack-internal:///./node_modules/.pnpm/next@13.0.3_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/client/components/app-router.js:74:11)
    at ErrorBoundaryHandler (webpack-internal:///./node_modules/.pnpm/next@13.0.3_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/client/components/error-boundary.js:28:9)
    at ErrorBoundary (webpack-internal:///./node_modules/.pnpm/next@13.0.3_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/client/components/error-boundary.js:40:11)
    at AppRouter
    at ServerRoot (webpack-internal:///./node_modules/.pnpm/next@13.0.3_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/client/app-index.js:113:11)
    at RSCComponent
    at Root (webpack-internal:///./node_modules/.pnpm/next@13.0.3_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/client/app-index.js:130:11)

And during production (hosted on Vercel), the following hydration errors seem to randomly appear regardless of how much I pair down the example so long as I'm using next-themes in this way:

Error: Minified React error #418; visit https://reactjs.org/docs/error-decoder.html?invariant=418 for the full message or use the non-minified dev environment for full errors and additional helpful warnings.
    at zh (796-3c8db907cc96f9a4.js:9:55756)
    at ...

145 looks to be a cookies-based approach to solving this issue, but the PR looks stalled and I'm not sure if it's a working solution or even the best approach. So I wanted to open up an issue here to track suggestions & progress.

Thanks!

litewarp commented 1 year ago

Can't speak to all the issues brought up, but the hydration errors occur because of the way that next/react streams the html. If you replace the <script> tag in the ThemeScript component with the <NextScript /> component, it should hydrate correctly.

WITS commented 1 year ago

145 was working with an earlier build of Next.js, but it definitely looks like using `<NextScript />` can clean that pr up quite a bit now. I'm gonna try to update it this week

transitive-bullshit commented 1 year ago

Thanks @WITS 🙏

@leerob would you be able to provide any guidance on the best solution here (or forward to someone who might)?

WITS commented 1 year ago

That pr should be updated to work with Next.js 13.0.3 now. I'm still looking into making further improvements, but you can test the changes through the npm package @wits/next-themes prior to the pr being merged. (Or try running npm i "next-themes@npm:@wits/next-themes" if you want to keep the same imports in your code.)

I'd like to provide some more context on why the current approach uses cookies. Unfortunately, trying to get this working in the app directory without ensuring that the server is rendering the same <html> attributes results in errors. So I reached out to @pacocoursey as well as some members of the Next.js team before starting this work, and they agreed that a cookie-based approach would be the right move.

next/script is a great call! It wasn't available for the app directory when I first started writing these changes, but it looks like using it with stategy="beforeInteractive" works here. However, doing so inside of a client component (like the existing ThemeProvider still causes it to run too late and the page will flicker un-themed content. Also, by default, that script relies on window.matchMedia to determine the system theme of the user. That isn't available on the server, so without cookies we still get errors due to a mismatch between client and server rendering during fast refreshes.

Update: Please make sure you use the <ServerThemeProvider> (as outlined here) if you're trying this out. If you want to have theme switcher UI, you'll need both the <ServerThemeProvider> in the layout and a <ThemeProvider> in an interactive ("use client") component.

Update 2: The fork above is no longer being maintained! Please see this comment for instructions on how to use the stable version of next-themes with appDir.

transitive-bullshit commented 1 year ago

Appreciate the in-depth breakdown @WITS.

I just tried @wits/next-themes on my project, and while I can verify it's taking effect, I'm not seeing any changes with regards to the error messages. On next dev, the same hydration error occurs w.r.t. extra html attributes, and on production, the same hydration errors appear sporadically (same as previously).

This could be a problem with my app, of course, but the hydration errors during next dev give me pause.

WITS commented 1 year ago

Would you be able to share your updated app/layout.tsx (or a simplified version of it) so I can reproduce the issue locally?

transitive-bullshit commented 1 year ago

Definitely not a minimal repro, but my source is here: https://github.com/transitive-bullshit/next-movie/blob/main/app/layout.tsx. See RootLayoutProviders.

I currently have my usage of next-themes commented out in the providers and in a few places in the code because no matter what I tried, I would get sporadic hydration errors on prod that would cause the entire client-side JS to stop working — resulting in a dead app for users. It should be easy to re-enable once next-themes is patched.

WITS commented 1 year ago

Ah, I should have clarified when I explained the approach above that it requires a new component. Can you try wrapping the <html> tag in your layout.tsx file with the <ServerThemeProvider> from @wits/next-themes and see if that works?

(There's a section in the updated README with an example.)

transitive-bullshit commented 1 year ago

Ahhh good catch.

Okay, so after doing that and re-enabling my usage of next-themes@npm:@wits/themes, I'm seeing the following.

Dev

During next dev the first few times it loads is fine, but as soon as I toggle dark mode client-side and hard refresh, I start seeing mismatched hydration warnings in the dev console:

Prod

It works well hydration-wise 💯 I haven't been able to reproduce any hydration errors when deployed as a preview build to Vercel: https://next-movie-hrwhglaz6-saasify.vercel.app/

However, I'm now seeing different, consistent 500 errors from Vercel using this branch with next-themes enabled that doesn't repro on main:

Error: invariant: invalid Cache-Control duration provided: 0 < 1

Example: https://next-movie-hrwhglaz6-saasify.vercel.app/titles/424

@WITS I'm going to hold off on re-enabling dark mode toggle in prod for now. I'm not too worried about the next dev errors, so long as it works on prod, though we should probably get them fixed before merging #145. If you want to test locally, check out https://github.com/transitive-bullshit/next-movie/tree/feature/next-themes-beta and set DATABASE_URL to a Postgres instance. You may also need to do npx prisma db push or npx prisma generate.

I think this cache-control issue invariant issue is likely a bug with next@canary, though it's strange that it only repros on this branch where the only changes involve enabling next-themes.

transitive-bullshit commented 1 year ago

I created a few Next.js issues to track the errors I'm running into. Not sure what else I can do to help.

willin commented 1 year ago

[0] event - compiled client and server successfully in 837 ms (350 modules)
[0] wait  - compiling /_error (client and server)...
[0] error - (sc_server)/node_modules/.pnpm/next-themes@0.2.1_7iuvftg57tblwyxclfkwku5xo4/node_modules/next-themes/dist/index.js (1:223) @ eval
[0] error - TypeError: e.createContext is not a function
[0]     at eval (webpack-internal:///(sc_server)/./node_modules/.pnpm/next-themes@0.2.1_7iuvftg57tblwyxclfkwku5xo4/node_modules/next-themes/dist/index.js:11:92)
[0]     at Object.(sc_server)/./node_modules/.pnpm/next-themes@0.2.1_7iuvftg57tblwyxclfkwku5xo4/node_modules/next-themes/dist/index.js (/Users/v0/Sites/alpha/.next/server/app/[locale]/page.js:345:1)
[0]     at __webpack_require__ (/Users/v0/Sites/alpha/.next/server/webpack-runtime.js:33:42)
[0]     at eval (webpack-internal:///(sc_server)/./app/[locale]/themes.tsx:11:69)
[0]     at Object.(sc_server)/./app/[locale]/themes.tsx (/Users/v0/Sites/alpha/.next/server/app/[locale]/page.js:313:1)
[0]     at __webpack_require__ (/Users/v0/Sites/alpha/.next/server/webpack-runtime.js:33:42)
[0]     at eval (webpack-internal:///(sc_server)/./app/[locale]/page.tsx:6:65)
[0]     at Object.(sc_server)/./app/[locale]/page.tsx (/Users/v0/Sites/alpha/.next/server/app/[locale]/page.js:292:1)
[0]     at Function.__webpack_require__ (/Users/v0/Sites/alpha/.next/server/webpack-runtime.js:33:42)
[0]     at processTicksAndRejections (node:internal/process/task_queues:96:5) {
[0]   type: 'TypeError',
[0]   page: '/[locale]'
[0] }

willin commented 1 year ago

That pr should be updated to work with Next.js 13.0.3 now. I'm still looking into making further improvements, but you can test the changes through the npm package @wits/next-themes prior to the pr being merged. (Or try running npm i "next-themes@npm:@wits/next-themes" if you want to keep the same imports in your code.)

I'd like to provide some more context on why the current approach uses cookies. Unfortunately, trying to get this working in the app directory without ensuring that the server is rendering the same <html> attributes results in errors. So I reached out to @pacocoursey as well as some members of the Next.js team before starting this work, and they agreed that a cookie-based approach would be the right move.

next/script is a great call! It wasn't available for the app directory when I first started writing these changes, but it looks like using it with stategy="beforeInteractive" works here. However, doing so inside of a client component (like the existing ThemeProvider still causes it to run too late and the page will flicker un-themed content. Also, by default, that script relies on window.matchMedia to determine the system theme of the user. That isn't available on the server, so without cookies we still get errors due to a mismatch between client and server rendering during fast refreshes.

[0] event - compiled client and server successfully in 655 ms (199 modules)
[0] wait  - compiling /middleware (client and server)...
[0] event - compiled successfully in 51 ms (67 modules)
[0] wait  - compiling /[locale]/page (client and server)...
[0] event - compiled client and server successfully in 440 ms (362 modules)
[0] TypeError: Cannot read properties of undefined (reading '_context')
[0]     at Object.useContext (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react/cjs/react.shared-subset.development.js:1418:29)
[0]     at exports.useTheme (webpack-internal:///(sc_server)/./node_modules/.pnpm/@wits+next-themes@0.2.12_7iuvftg57tblwyxclfkwku5xo4/node_modules/@wits/next-themes/dist/index.js:215:28)
[0]     at ChangeTheme (webpack-internal:///(sc_server)/./app/[locale]/themes.tsx:147:84)
[0]     at attemptResolveElement (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1207:42)
[0]     at resolveModelToJSON (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1660:53)
[0]     at Object.toJSON (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1121:40)
[0]     at stringify (<anonymous>)
[0]     at processModelChunk (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:172:36)
[0]     at retryTask (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1868:50)
[0]     at performWork (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1906:33)
[0] TypeError: Cannot read properties of undefined (reading '_context')
[0]     at Object.useContext (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react/cjs/react.shared-subset.development.js:1418:29)
[0]     at exports.useTheme (webpack-internal:///(sc_server)/./node_modules/.pnpm/@wits+next-themes@0.2.12_7iuvftg57tblwyxclfkwku5xo4/node_modules/@wits/next-themes/dist/index.js:215:28)
[0]     at ChangeTheme (webpack-internal:///(sc_server)/./app/[locale]/themes.tsx:147:84)
[0]     at attemptResolveElement (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1207:42)
[0]     at resolveModelToJSON (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1660:53)
[0]     at Object.toJSON (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1121:40)
[0]     at stringify (<anonymous>)
[0]     at processModelChunk (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:172:36)
[0]     at retryTask (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1868:50)
[0]     at performWork (webpack-internal:///(sc_server)/./node_modules/.pnpm/next@13.0.5_biqbaboplfbrettd7655fr4n2y/node_modules/next/dist/compiled/react-server-dom-webpack/server.browser.js:1906:33) {
[0]   digest: '3476920131'
[0] }

dawidseipold commented 1 year ago

@WITS Hi, I am using the @wits/next-themes ServerThemeProvider, but I still get the same hydration error. Not sure what the problem might be. Here's the code snippet (it's a fresh project).

const RootLayout = ({ children }: IProps) => {
  return (
    <ServerThemeProvider>
      <html lang="en">
        <head />

        <Providers>
          <body className="bg-red-500 dark:bg-blue-500">{children}</body>
        </Providers>
      </html>
    </ServerThemeProvider>
  );
};

After adding attribute='class' props to the provider the hydration still error persists, but is displayed after this error Cannot read properties of null (reading 'textContent'). Any help would be appreciated :)

WITS commented 1 year ago

I published a new version of @wits/next-themes (0.2.13) today that includes some fixes.

@transitive-bullshit Good catch! The "e" value was due to a bug in my pr, and should be fixed now. However, I can still reproduce the hydration issues whenever I explicitly set the theme to something other than system. I think this might be an issue with Next caching the first value it receives for the cookie after the server starts. I'm going to investigate further and reach out to them if that's the case.

@willin Is your project using <ServerThemeProvider> in the layout?

@dawidseipold Are you using the same properties on your <ServerThemeProvider> as your <ThemeProvider>? (If you're setting attribute="class" on one, it needs to be on both.)

dawidseipold commented 1 year ago

@WITS Yep! The hydration error is doubled if I do that. Having only <ServerThemeProvider> gives me one error.

WITS commented 1 year ago

Okay, just published 0.2.14 which should fix more hydration errors. (Maybe all of them? 🤞)

willin commented 1 year ago

Okay, just published 0.2.14 which should fix more hydration errors. (Maybe all of them? 🤞)

nope,

RootLayout:

      <ServerThemeProvider attribute='class' themes={themes.map((x) => x.id)}>
        <html lang={locale}>
          <head />
          <body>
            <div
              id='background'
              className={clsx({
                dark: true //darkThemes.map((x) => x.toLowerCase()).includes(theme)
              })}></div>
            {/* <Header /> */}
            <div className='pt-20' style={{ minHeight: 'calc(100vh - 75px)' }}>
              {children}
            </div>
            <Bootstrap />
          </body>
        </html>
      </ServerThemeProvider>

Component:

current theme none, set theme not work.

willin commented 1 year ago

if a nest a ThemeProvider inside ServerThemeProvider:

willin commented 1 year ago

is there a working example using class?

litewarp commented 1 year ago

Hey @WITS, I can get the page to render using both <ServerThemeProvider /> and a client <ThemeProvider/>, but I can't get setTheme to work. I'll try and reproduce it for you. In the meantime, I noticed that when using both providers, the script gets injected twice. Not sure if this would be the cause of some issues.

dawidseipold commented 1 year ago

@willin same problem when I nest. It also shows in the console that the classes on html element are different.

balthazar commented 1 year ago

Hey @WITS, I haven't managed to get your 0.2.14 to properly function (with the ServerProvider and ThemeProvider), but it might be because I'm on Next 13.0.6?

Warning: Cannot render a sync or defer Githubissues.
Githubissues is a development platform for aggregating issues.

pacocoursey / next-themes

Next.js 13 appDir support #152

145 looks to be a cookies-based approach to solving this issue, but the PR looks stalled and I'm not sure if it's a working solution or even the best approach. So I wanted to open up an issue here to track suggestions & progress.

145 was working with an earlier build of Next.js, but it definitely looks like using `<NextScript />` can clean that pr up quite a bit now. I'm gonna try to update it this week

Dev

Prod

pacocoursey / next-themes

Next.js 13 appDir support #152

145 looks to be a cookies-based approach to solving this issue, but the PR looks stalled and I'm not sure if it's a working solution or even the best approach. So I wanted to open up an issue here to track suggestions & progress.

145 was working with an earlier build of Next.js, but it definitely looks like using <NextScript /> can clean that pr up quite a bit now. I'm gonna try to update it this week

Dev

Prod

145 was working with an earlier build of Next.js, but it definitely looks like using `<NextScript />` can clean that pr up quite a bit now. I'm gonna try to update it this week