next.js icon indicating copy to clipboard operation
next.js copied to clipboard

CSS module styling is removed too early on route changes

Open cmwall opened this issue 5 years ago • 89 comments
trafficstars

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

  • OS: macOS
  • Browser (if applies): N/A
  • Version of Next.js: 9.5.3
  • Version of Node.js: 12.14.1

cmwall avatar Sep 30 '20 04:09 cmwall

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 avatar Oct 03 '20 09:10 MihirGH

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 avatar Oct 03 '20 10:10 MihirGH

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

MihirGH avatar Oct 03 '20 11:10 MihirGH

Hi, experiencing the very same issue as reported.

ccambournac avatar Oct 12 '20 12:10 ccambournac

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.

scriptify avatar Oct 18 '20 17:10 scriptify

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

tommhuth avatar Oct 22 '20 08:10 tommhuth

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

MihirGH avatar Oct 22 '20 17:10 MihirGH

@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?

scriptify avatar Oct 23 '20 06:10 scriptify

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.

tommhuth avatar Oct 23 '20 08:10 tommhuth

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

scriptify avatar Oct 23 '20 12:10 scriptify

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

Limekiller avatar Oct 29 '20 16:10 Limekiller

@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

fredcorr avatar Oct 29 '20 19:10 fredcorr

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

Limekiller avatar Oct 29 '20 19:10 Limekiller

@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.

scriptify avatar Oct 30 '20 12:10 scriptify

@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?

fredcorr avatar Oct 31 '20 12:10 fredcorr

@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 🤗

scriptify avatar Oct 31 '20 16:10 scriptify

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.

tommhuth avatar Nov 09 '20 23:11 tommhuth

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.

OlivierFortier avatar Nov 27 '20 21:11 OlivierFortier

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

controversial avatar Dec 02 '20 07:12 controversial

@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.

timneutkens avatar Dec 02 '20 14:12 timneutkens

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

cmwall avatar Dec 02 '20 23:12 cmwall

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

nayarahilton avatar Dec 03 '20 16:12 nayarahilton

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

controversial avatar Dec 16 '20 00:12 controversial

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

ShuPink avatar Dec 17 '20 02:12 ShuPink

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

leo-cheron avatar Dec 18 '20 13:12 leo-cheron

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.

stefanmaric avatar Dec 25 '20 16:12 stefanmaric

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.

wushaobo avatar Dec 30 '20 10:12 wushaobo

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

controversial avatar Jan 02 '21 20:01 controversial

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

ccambournac avatar Jan 10 '21 10:01 ccambournac

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.

controversial avatar Jan 10 '21 18:01 controversial