vercel / next.js

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

CSS module removed when navigating pages that contain the same next/dynamic component #16950

Closed adamayres closed 4 years ago

adamayres commented 4 years ago

Bug report

Describe the bug

Components that use next/dynamic to load and that have a CSS module lose their CSS when navigating to another page that contains the same dynamically loaded component.

To Reproduce

Here is a repo that contains the minimal amount of code to reproduce.

  1. Create a Component that includes a CSS module.
  2. Create two pages.
  3. Using next/dynamic add the component to each page, as well as add a link to the other page.
  4. Start the app and view the first page and then click the link to the second page.

Expected behavior

The styles for the dynamically included component will persist when navigating to other pages the component lives on.

Screenshots

The styles for the dynamically included component do not persist when navigating to other pages the component lives on.

System information

Additional context

This issue only occurs when navigating via page links, direct access to the index or other-page load the CSS correctly.

This bug appears to have been introduced as a regression from #12843 that fixed a related issue, #10557, where there is a flash of unstyled content when navigating to a page that contains a dynamically loaded component. I verified that this new issue is not present in 9.5.3-canary.20, but is present on 9.5.3-canary.21 and is currently present in 9.5.3. See this comment thread for more details on potential solutions.

diegoleft commented 4 years ago

Same problem here Next version 9.5.3

khades commented 4 years ago

@timneutkens @Timer there's two solutions

  1. Explose react-loadable-manifest to client-loader so it would properly know that some specific css files should be stored between page transitions. This might fully replace mini-css-extract-plugin way of importing such css files.
  2. Dont unload css files that were requested by dynamic component.

@adamayres

I would defer to the maintainers of the library to provide guidance on which of these approaches makes the most sense for the framework, however I do see that the react-loadable-manifest.json created by the build of my application is ~1.3 MBs raw and ~79kb gzipped, which seems like too much overheard to send to the client to solve this problem. Perhaps there is someway to only send the necessary information about the components being used for a given page OR perhaps it is acceptable to leave CSS on a page for the dynamically loaded components.

Not possible

diegoleft commented 4 years ago

Just to add new info:

This is the normal view of one page (when you refresh or access first time)

Captura de Pantalla 2020-09-09 a la(s) 09 33 50

This is the view with the issue (when you enter after navigation) You can see is missing one chunk CSS file, in my specific case, the missing CSS is that built to replace the original LESS of Ant.design

Captura de Pantalla 2020-09-09 a la(s) 09 34 02
adamayres commented 4 years ago

Perhaps there is someway to only send the necessary information about the components being used for a given page

Not possible

@khades Regarding option two of not unloading the CSS, is the main concern that the CSS on the client will continue to grow on subsequent page loads and in some cases there will be CSS on the page for a dynamically loaded component that is no longer needed? Are there other concerns of not unloading the CSS for dynamically loaded components?

If there are no other concerns, then it seems like the trade off between your original two options is with option 1 there is potentially a large asset that is required to be downloaded on the client, vs option 2 where there is a potentially growing amount of CSS across page loads that is potentially not needed.

With option 1 could we make it such that the manifest asset does not block the first page load and set it to be prefetched/preloaded for use by subsequent page views that need this information? Could we also make it such that on subsequent page views, if this manifest is still not completely downloaded, we allow the page to continue to render and cleanup any unused CSS once the manifest is available? If so, then I would lean towards option 1, otherwise I would go with option 2.

EDIT: This tradeoff feels like it is something that may varying based on usage patterns, where applications with large number of components may have very large manifests. One consideration might be to set a threshold where if gzipped manifest exceeds a (configurable) size then option 2 kicks in. Or perhaps another option is to make it configurable so individuals can choose based on their specific usage pattern. The downside of these suggestions is complexity.

Gr0m commented 4 years ago

Same (or maybe similar/related) problem. I'm using "next-page-transitions" package in _app.tsx to make page transitions. With every page transition, parts of CSS (I'm using css modules) are removed immediately (instead of waiting for transition to finish) and the page looks really weird for transition duration.

Forgot to say that this is happening only on production. And it doesn't happen on 9.5.2.

khades commented 4 years ago

@timer i can write code so dynamically added css files wont be unloaded on page transition, is that a good idea?

khades commented 4 years ago

Well i feel that leaving old CSS module files from dynamic components since the mechanism of their insertion was NOT handled by next itself.

Code that it was generated before was like

image

Basically mini-css-extract-plugin added hardcoded stylesheets into html document when specific chunk appear. After that it was NOT cleared by client (packages\next\client\index.tsx). Right now it is included in clearup logic.

So i think it would be safe to stop that styles from being cleared.

balazsorban44 commented 2 years ago

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.