vercel / next.js

The React Framework
https://nextjs.org
MIT License
125.82k stars 26.85k forks source link

CSS module styling is removed too early on route changes #17464

Closed cmwall closed 6 months ago

cmwall commented 4 years ago

Bug report

Describe the bug

CSS module styling is removed immediately after clicking a next/link, instead of after the DOM is removed on production builds. This causes the components to have no styling at all during a page transition. This issue does not happen on dev mode.

I believe this is a bug with CSS modules specifically because components styled with styled-jsx don't have this problem.

Really would love to be able to use Sass via CSS modules here instead of re-writing the entire app I'm working on using styled-jsx. If Sass modules can't work in this scenario, I think I would be forced to use styled-jsx, which is not my preferred method of styling my components for this project.

To Reproduce

I have created repos, and deployed these repos to demonstrate the problem using framer-motion for page transitions. If you were to pull these repos and run them locally using npm run dev, you will see that the flash of unstyled content does not happen on any one of them in dev mode. However, on their deployed sites, you can see the flash of unstyled content with CSS modules and Sass modules.

styled-jsx

Behavior: correct, no flash of unstyled content Deployed site on Vercel Repo

CSS modules

Behavior: buggy, there is a flash of unstyled content immediately after clicking the link Deployed site on Vercel Repo

Sass via CSS modules (additional)

Behavior: buggy, there is a flash of unstyled content immediately after clicking the link (same as CSS modules) Deployed site on Vercel Repo

Expected behavior

Styling for components that come from CSS modules should not be removed immediately on route changes, and instead, are removed when the markup is removed (the component unmounts?). The expected behavior is the behavior we can see on the styled-jsx deployment above.

System information

NEXT-1351

MihirGH commented 4 years ago

I looked at this issue and this seems to be happening because of the behaviour implemented in onCommit method.

Inside onCommit, we are setting the media attribute to the style tags which are not desired too early.

Exploring this further and will raise a PR for this in sometime

MihirGH commented 4 years ago

Essentially what I ended up finding was: we should never set media attribute to x on the <style> tags since they might still be in use even though the page that was using it is being replaced. To do this, we simply need to get rid of the code that is executing in the else block of this loop in onCommit.

I don't know if this was done because of any specific reasons or not but this seems to solve the issue on my local after making these changes.

@lfades any thoughts? If it looks good to you then I'll go ahead and raise a PR for this

MihirGH commented 4 years ago

Also, to make this work and to have the <style> tags on SSRed page, we might need to remove the isInitialRender check in onStart and !isInitialRender check in onCommit

ccambournac commented 4 years ago

Hi, experiencing the very same issue as reported.

scriptify commented 3 years ago

Experiencing the same issue. I also noticed the media attribute being set to x on the relevant style tags. Would love to avoid the switch to styled-jsx, as it doesn't play well with framer motion: https://github.com/framer/motion/issues/231

I've applied this hack in the mean time:

    // Add that code to _app.tsx / _app.jsx

    import Router from "next/router";

    const routeChange = () => {
      // Temporary fix to avoid flash of unstyled content
      // during route transitions. Keep an eye on this
      // issue and remove this code when resolved:
      // https://github.com/vercel/next.js/issues/17464

      const tempFix = () => {
        const allStyleElems = document.querySelectorAll('style[media="x"]');
        allStyleElems.forEach((elem) => {
          elem.removeAttribute("media");
        });
      };
      tempFix();
    };

   Router.events.on("routeChangeComplete", routeChange );
   Router.events.on("routeChangeStart", routeChange );

I am not sure what other unwished side effects this temporary fix could have, but it seems to work just well enough for my application.

tommhuth commented 3 years ago

I'm having the same issue, but @scriptify's fix does not work for me, any ETA for a fix for this? @MihirGH

MihirGH commented 3 years ago

@tommhuth It is actually a quick fix but I am not sure if that's how it is supposed to be solved or not.

scriptify commented 3 years ago

@MihirGH That code is just a temporary workaround, it also possible that Next.js is setting the media attribute slightly after those events fire, maybe that's the reason why it's not working for @tommhuth. @tommhuth are you sure that in your case the problem is also related to the media attribute?

tommhuth commented 3 years ago

Having looked into this, I can't see anywhere that media attribute is being changed, but I do see that the relevant link has its rel set from stylesheet to preload. If I manually revert it to stylesheet the appropriate styles are back, but when I do this in a routeChangeComplete/routeChangeComplete event callback there is a flash of unstyled content so those events are probably not accurate enough to serve as a quick fix. Also, I don't think I have any way of knowing exactly what style link is needed, so I end up setting all links with css files back to stylesheet which might be unnecessary. @scriptify @MihirGH

Off the top of my head, the only way I could get around this is by moving all style modules into non-module SCSS, but that is quite a significant workaround.

scriptify commented 3 years ago

Hmm that's strange, seems like a severe issue to me, and moving everything into non modular CSS seems like a very tedious task, just to make it modular again when it's fixed (besides the whole disadvantages global CSS has). Are we the only ones using CSS Modules + non-instant route changes? 🤷‍♂️ Doesn't seem too exotic to me

Limekiller commented 3 years ago

@scriptify I was running into this issue as well; your hack worked for me for the time being, so thanks for that.

fredcorr commented 3 years ago

@scriptify I bumped into the same issue. could you help me understand where I would add the fix you wrote? Thanks for your help as well

Limekiller commented 3 years ago

@fredcorr I put the code in _app.js along with, of course, import Router from 'next/router'

scriptify commented 3 years ago

@fredcorr Yea that code is a bit out of context, as @Limekiller mentioned, the best place to put it is _app.tsx / _app.jsx. I updated my comment accordingly to clear that up.

fredcorr commented 3 years ago

@Limekiller @scriptify thanks guys that fixed the issue partially, on the first-page transition the issue still occurs. Are you guys using the getStaticProps or GgetServerSideProps? Could that maybe affect it? @MihirGH Any updated on how long will it take to fix this?

scriptify commented 3 years ago

@fredcorr I'm using getStaticProps for all my pages for this particular project, but I don't think that should affect the route transition behaviour, just speculating ofc. Would be nice to get some hints from the Next.js team 🤗

tommhuth commented 3 years ago

For anyone needing a dirty quick fix for this one until a fix is released, simply importing the modules whose style is needed in _app.js solved it for me without requiring any other refactoring.

OlivierFortier commented 3 years ago

I am having the exact same problem, and @scriptify 's fix seemed to have resolved the issue for me in general.

However it still has the same issue but only on the first page transition (regardless of which page) , afterwards every page transition to any page in any order seems fine. I guess that will have to do until an official fix comes.

controversial commented 3 years ago

@Timer I can confirm that #19125 didn’t fix this, as I’m still seeing this issue on 10.0.3 :(

timneutkens commented 3 years ago

@Timer I can confirm that #19125 didn’t fix this, as I’m still seeing this issue on 10.0.3 :(

Would be useful if you provide a reproduction for your particular case.

cmwall commented 3 years ago

@timneutkens Would it help if I updated the original demonstration repos to 10.0.3?

nayarahilton commented 3 years ago

@scriptify thanks for the partial solution! Some thoughts in how to fix that for the first-page transition?

controversial commented 3 years ago

Would be useful if you provide a reproduction for your particular case.

I think the original reproduction from @cmwall attached to this issue represents my use case, but I can deploy another copy with version 10.0.3 if that helps

ShuPink commented 3 years ago

Hiya, I've also had this problem on 10.0.3, using react-transition-group and css modules.

@timneutkens I've tried my hand at a reproduction here. If you go from index-> page 1, you'll see the styles be removed from the index page for second. If you go from page 1 -> page 2 there won't be an issue because the same stylesheet is used.

If you run this in dev the transitions work as expected.

The repo is here Um. Let me know if the demo isn't clear.

Here's a screenshot of the index page when it exits and it's styles are removed during transition

Vs. pre-transition pre-transition

leo-cheron commented 3 years ago

This issue is quite old and breaks every page transitioning website since nextjs 9.3. Shouldn't it be moved to the next iteration?

stefanmaric commented 3 years ago

I've encounter this issue as well and tried @scriptify 's suggested woraround but—as other have mentioned—it didn't seem to really help with the first navigation.

After looking deeper, I realized this is a hard problem to solve for the framework. I don't really know how pre-v10 versions of Next.js worked in this regard since the project I'm working on started with the v10, but I can assume any page transition solution relied on a bug or an unoptimized behavior.

When the page changes you want to remove styles (and maybe other kind of resources) from the DOM to 1) prevent style clashes (in the case of global CSS) and to 2) prevent memory leaks as the CSSOM would otherwise increase on every page navigation.

Next.js handles this well.

The problem with page transitions arises from the fact that any solution (being it with Framer motion, ReactTransitionGroup, ReactFlipToolkit, etc) relies on holding to the previous render's element tree (its children) until the animation has finished.

Next.js cannot know when such animation finishes, instead, it has to unlink styles from the DOM as soon as the current (old) page Component is replaced by the next (new) page Component.

I don't think here's any "fix" Next.js can apply for this. The only solution I can think of is for Next.js to provide a new set of APIs to hook to and manipulate its resource management system on the client-side. Think of providing callbacks on the _app interface or making the Route events an async middleware that allows one to delay the next step in the route change process.

tl;dr: a workaround

I've come up with a workaround that 1) seems to be working even for the first navigation and 2) cleans up the DOM after the page transition has finished.

There's 2 variations depending on the animation system you use. Note you would use one or the other depending on your case, not both.

Spring-based

The first one is for spring-based animations which provide a completion callback. Place this hook anywhere you prefer:

// utils/useTransitionFix.ts

import Router from 'next/router'
import { useCallback, useEffect, useRef } from 'react'

type Cleanup = () => void

export const useTransitionFix = (): Cleanup => {
  const cleanupRef = useRef<Cleanup>(() => {})

  useEffect(() => {
    const changeListener = () => {
      // Create a clone of every <style> and <link> that currently affects the page. It doesn't
      // matter if Next.js is going to remove them or not since we are going to remove the copies
      // ourselves later on when the transition finishes.
      const nodes = document.querySelectorAll('link[rel=stylesheet], style:not([media=x])')
      const copies = [...nodes].map((el) => el.cloneNode(true) as HTMLElement)

      for (let copy of copies) {
        // Remove Next.js' data attributes so the copies are not removed from the DOM in the route
        // change process.
        copy.removeAttribute('data-n-p')
        copy.removeAttribute('data-n-href')

        // Add duplicated nodes to the DOM.
        document.head.appendChild(copy)
      }

      cleanupRef.current = () => {
        for (let copy of copies) {
          // Remove previous page's styles after the transition has finalized.
          document.head.removeChild(copy)
        }
      }
    }

    Router.events.on('beforeHistoryChange', changeListener)

    return () => {
      Router.events.off('beforeHistoryChange', changeListener)
      cleanupRef.current()
    }
  }, [])

  // Return an fixed reference function that calls the internal cleanup reference.
  return useCallback(() => {
    cleanupRef.current()
  }, [])
}

Then you can use it in your app, e.g.:

// pages/_app.ts

import { AnimatePresence, motion } from 'framer-motion'
import type { AppProps } from 'next/app'

import { useTransitionFix } from '../utils/useTransitionFix'

const PAGE_VARIANTS = {
  initial: {
    opacity: 0,
  },
  animate: {
    opacity: 1,
  },
  exit: {
    opacity: 0,
  },
}

function App({ Component, pageProps, router }: AppProps): React.ReactElement {
  const transitionCallback = useTransitionFix()

  return (
      <AnimatePresence exitBeforeEnter onExitComplete={transitionCallback}>
        <motion.div
          key={router.route}
          initial="initial"
          animate="animate"
          exit="exit"
          variants={PAGE_VARIANTS}
        >
          <Component {...pageProps} />
        </motion.div>
      </AnimatePresence>
  )
}

export default App

Timeout-based

The second one is for solutions that use fixed-duration transitions:

// utils/fixTimeoutTransition.ts

import Router from 'next/router'

export const fixTimeoutTransition = (timeout: number): void => {
  Router.events.on('beforeHistoryChange', () => {
    // Create a clone of every <style> and <link> that currently affects the page. It doesn't matter
    // if Next.js is going to remove them or not since we are going to remove the copies ourselves
    // later on when the transition finishes.
    const nodes = document.querySelectorAll('link[rel=stylesheet], style:not([media=x])')
    const copies = [...nodes].map((el) => el.cloneNode(true) as HTMLElement)

    for (let copy of copies) {
      // Remove Next.js' data attributes so the copies are not removed from the DOM in the route
      // change process.
      copy.removeAttribute('data-n-p')
      copy.removeAttribute('data-n-href')

      // Add duplicated nodes to the DOM.
      document.head.appendChild(copy)
    }

    const handler = () => {
      // Emulate a `.once` method using `.on` and `.off`
      Router.events.off('routeChangeComplete', handler)

      window.setTimeout(() => {
        for (let copy of copies) {
          // Remove previous page's styles after the transition has finalized.
          document.head.removeChild(copy)
        }
      }, timeout)
    }

    Router.events.on('routeChangeComplete', handler)
  })
}

Which you would use outside of of your app component:

// pages/_app.ts
import { CSSTransition, TransitionGroup } from "react-transition-group"
import type { AppProps } from 'next/app'

import { fixTimeoutTransition } from '../utils/useTransitionFix'

import '../styles/globals.css'

const TRANSITION_DURATION = 500

fixTimeoutTransition(TRANSITION_DURATION)

function App({ Component, pageProps, router }: AppProps): React.ReactElement {
  return (
    <TransitionGroup>
      <CSSTransition
        classNames="app-transition-wrapper"
        enter
        exit
        key={router.asPath}
        timeout={TRANSITION_DURATION}
        unmountOnExit
      >
        <Component {...pageProps} />
      </CSSTransition>
    </TransitionGroup>
  )
}

export default App

So far I haven't found issues with these. Hope it helps you all.

wushaobo commented 3 years ago

It is quite a scary news for my team, since we just started to switch from styled-components to css modules a week ago. Has it been fixed since 10.0.4? Is there a rough estimation time of release in your plan? Please kindly update. Thanks.

controversial commented 3 years ago

This is the most upvoted open bug on this repo right now

ccambournac commented 3 years ago

@stefanmaric Thanks for the workaround! In my case, I had to use the routeChangeStart event for the spring-based workaround to work.

controversial commented 3 years ago

Worth noting that in addition to styles being removed, the page’s scroll position also jumps to the top at the beginning of the transition.

ccambournac commented 3 years ago

Still experiencing small blinks on first navigation... https://www.loom.com/share/ee94e6065c2b4ba488601176c39323d4

(But no scroll position jump @controversial)

ClayCooperLA commented 3 years ago

I fixed this by importing the component that is leaving into the _app.js file.

In my particular case, we only have one component that exposes the issue, so simply importing that component into app.js, not even rendering it, fixes the issue. This will probably work for other cases, you would just import any component that loses its styles during a transition into the _app.js file. Obviously you will lose some code splitting magic, but in our case, we feed optional data into the components so it's really just an empty version of the component that is being imported with its styles.

Timer commented 3 years ago

@stefanmaric has explained this situation quite well.

tl;dr Next.js handles CSS removal and injection correctly when no libraries "hold onto" previous rendering trees.

I've included the relevant bit here:

The problem with page transitions arises from the fact that any solution (being it with Framer motion, ReactTransitionGroup, ReactFlipToolkit, etc) relies on holding to the previous render's element tree (its children) until the animation has finished.

Next.js cannot know when such animation finishes, instead, it has to unlink styles from the DOM as soon as the current (old) page Component is replaced by the next (new) page Component.

Next.js cannot simultaneously allow rendering with both sets of styles (from two separate pages) on screen, as this may result in styles conflicts which would trigger reflows or the incorrect styling of your application.

FWIW, this works with styled-jsx because the styles are defined as part of the render-tree instead of as a module side-effect.

It'd be great to see one of these libraries get the computed styles of each element and apply them as inline instead of thinking they'd never be removed from the page.


I've marked this as good first issue for someone to investigate further, as it'd probably be good to have a story around this.

Gr0m commented 3 years ago

Why was it working with 9.5.2? Don't want to stick with this version forever.

Timer commented 3 years ago

9.5.2 and earlier had numerous bugs with styling conflicts that we fixed (not sure this is the exact version, but going off what you said), as mentioned above:

Next.js cannot simultaneously allow rendering with both sets of styles (from two separate pages) on screen, as this may result in styles conflicts which would trigger reflows or the incorrect styling of your application.

To expand on this, we used to treat styles across pages as "append-only" which broke applications in all sorts of ways. By removing old styles, we closed over half a dozen issues with tons of upvotes. Unfortunately, fixing this issue had the side effect of revealing a bug/lack-of-feature in these transition libraries that do not snapshot styles.

Gr0m commented 3 years ago

That's unfortunate. But what you say makes sense.

So styled-jsx will still work. But what about Styled Components? Or will it fail the same way as CSS Modules do? Or maybe it will be better to use one of @stefanmaric solutions/workarounds?

I can leave my old projects on 9.5.2, but need to have a working solution for future projects.

I really like Next.js and don't want to use anything else (I have tried - didn't like :) )

Timer commented 3 years ago

Styled Components and Styled-JSX should both work fine with transition libraries. We're open to someone doing research into what it'd take to make styles append-only again, or if it's even possible without conflicts.

claudiobonfati commented 3 years ago

The @stefanmaric suggestion is a good fix for the most cases, but it causes a serious memory leak when a dynamic page change within the same route structure. For example, if you have a route /profile/[username], and you switch pages within the same route, like from /profile/vader to /profile/yoda the Cleanup function will not be triggered, which will clone the CSS nodes from DOM over and over again (4 nodes will be 8, 8 will be 16, 16 to 32, 32 to 64, 128, 256, 512...)

claus commented 3 years ago

[[[ THIS IS BUGGY AND OUTDATED, SEE UPDATE ]]]

Here's a hack that basically restores the Next 9.5 behavior in Next 10 (all page styles are preserved).

useEffect(() => {
    Array.from(
        document.querySelectorAll('head > link[rel="stylesheet"][data-n-p]')
    ).forEach(node => {
        node.removeAttribute('data-n-p');
    });
    const mutationHandler = mutations => {
        mutations.forEach(({ target }) => {
            if (target.nodeName === 'STYLE') {
                if (target.getAttribute('media') === 'x') {
                    target.removeAttribute('media');
                }
            }
        });
    };
    const observer = new MutationObserver(mutationHandler);
    observer.observe(document.head, {
        subtree: true,
        attributeFilter: ['media'],
    });
    return () => {
        observer.disconnect();
    };
}, []);

On mount, it removes the data-n-p attribute of the <link rel="stylesheet" data-n-p> elements so that Next doesn't remove those elements anymore. It also listens to changes in the media attributes of the page <style>s that are added when navigating to a new page. When a media="x" is added, it removes it again straight away.

Pro: Next won't remove any styles anymore Con: Next won't remove any styles anymore (watch out for possible conflicts)

timohofmeijer commented 3 years ago

In response to stefanmaric’s fix:

So far I haven't found issues with these. Hope it helps you all.

Running this fix in NODE_ENV 'development' caused serious issues for me (the react-native ipad app wrapping my next app crashing consistently), so I had to tweak it to only run in 'production'. Perhaps this info can save someone some time.

abdulrauf11 commented 3 years ago

Has this issue been fixed yet? 😢

satazor commented 3 years ago

Unfortunately not, at least in the latest released Next.js version.

I’ve made a solution based on @claus approach that handles duplicate styles and has been working well for us: https://github.com/moxystudio/next-with-moxy/blob/master/www/app/use-fouc-fix.js

mattvb91 commented 3 years ago

Not sure if this is related to the same thing but im currently experiencing css not being rendered on route changes on the next page if the css is imported for the component from node_modules. When I hard refresh for SSR it works fine

adedaniel commented 3 years ago

I had been battling with this issue for weeks now, occasionally when I change the route using Router.push, the css in the new page breaks.

I fixed this by using scriptify's answer. It seems to have fixed the problem. 🙂

SalmanK81099 commented 3 years ago

Experiencing the same issue. I also noticed the media attribute being set to x on the relevant style tags. Would love to avoid the switch to styled-jsx, as it doesn't play well with framer motion: framer/motion#231

I've applied this hack in the mean time:

    // Add that code to _app.tsx / _app.jsx

    import Router from "next/router";

    const routeChange = () => {
      // Temporary fix to avoid flash of unstyled content
      // during route transitions. Keep an eye on this
      // issue and remove this code when resolved:
      // https://github.com/vercel/next.js/issues/17464

      const tempFix = () => {
        const allStyleElems = document.querySelectorAll('style[media="x"]');
        allStyleElems.forEach((elem) => {
          elem.removeAttribute("media");
        });
      };
      tempFix();
    };

   Router.events.on("routeChangeComplete", routeChange );
   Router.events.on("routeChangeStart", routeChange );

I am not sure what other unwished side effects this temporary fix could have, but it seems to work just well enough for my application.

Hi, this code works perfectly but not on the first load I managed to get it working by adding the following in the app component.

React.useEffect(() => {
    router.push(router.pathname);
  }, []);
jacobbroughton commented 3 years ago

React.useEffect(() => { router.push(router.pathname); }, []);

This worked great for me but refreshing the page on a dynamic route gave me an error to the effect of 'Link component interpolation error', adding the query value and passing all current queries to the router.push inside of the useeffect fixed it for me. Hopefully this'll come in handy for someone!

router.push({
  pathname: router.pathname,
  query: {...router.query}
});
SalmanK81099 commented 3 years ago

React.useEffect(() => { router.push(router.pathname); }, []);

This worked great for me but refreshing the page on a dynamic route gave me an error to the effect of 'Link component interpolation error', adding the query value and passing all current queries to the router.push inside of the useeffect fixed it for me. Hopefully this'll come in handy for someone!

router.push({
  pathname: router.pathname,
  query: {...router.query}
});

I got to face the same problem but don't know why this solution did not work for me, Since on the first render of a dynamic link the router.pathname returns /[slug] or any param you passed instead of the actual param value so it will give you an interpolation error to fix it simply replace router.pathname by javascripts window.location.pathname which gives us the exact path and thus solves the problem without requiring to pass any queries.

React.useEffect(() => {
    router.push(window.location.pathname);
  }, []);

This will work with all types of links

yosefbeder commented 3 years ago

Here's a hack that basically restores the Next 9.5 behavior in Next 10 (all page styles are preserved).

useEffect(() => {
    Array.from(
        document.querySelectorAll('head > link[rel="stylesheet"][data-n-p]')
    ).forEach(node => {
        node.removeAttribute('data-n-p');
    });
    const mutationHandler = mutations => {
        mutations.forEach(({ target }) => {
            if (target.nodeName === 'STYLE') {
                if (target.getAttribute('media') === 'x') {
                    target.removeAttribute('media');
                }
            }
        });
    };
    const observer = new MutationObserver(mutationHandler);
    observer.observe(document.head, {
        subtree: true,
        attributeFilter: ['media'],
    });
    return () => {
        observer.disconnect();
    };
}, []);

On mount, it removes the data-n-p attribute of the <link rel="stylesheet" data-n-p> elements so that Next doesn't remove those elements anymore. It also listens to changes in the media attributes of the page <style>s that are added when navigating to a new page. When a media="x" is added, it removes it again straight away.

Pro: Next won't remove any styles anymore Con: Next won't remove any styles anymore (watch out for possible conflicts)

Thanks that helped me a lot

natashajaved commented 3 years ago

Hey, I am adding a css file using in a specific page, I'm running into a flashing issue, where the page appears without the new css and then styling appears, It only happens when I'm navigating to the page from some other page and not when I directly land on the page, tried @scriptify solution, it doesn't change anything. Appreciate any help. Adding link in head <Head> <link rel="stylesheet" href="/style-quran.css" /> </Head>

kevinwolfcr commented 3 years ago

Same happening for me, specifically when trying to do unmount transitions with framer-motion:

const App: React.FC<AppProps> = ({ Component, pageProps }) => {
  const router = useRouter()

  return (
    <AnimatePresence exitBeforeEnter>
      <Component key={router.asPath} {...pageProps} />
    </AnimatePresence>
  )
}