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

CSS Styles from unrelated layouts are merged if they have one CSS file in common #64773

Closed
onigoetz opened this issue Apr 19, 2024 · 16 comments · Fixed by #67373
Closed

CSS Styles from unrelated layouts are merged if they have one CSS file in common #64773

onigoetz opened this issue Apr 19, 2024 · 16 comments · Fixed by #67373
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked

Comments

@onigoetz
Copy link

onigoetz commented Apr 19, 2024

Link to the code that reproduces this issue

https://github.com/onigoetz/nextjs-css-issue-repro

To Reproduce

  1. build the app and start it (not reproducible in dev mode)
  2. go to http://localhost:3000/
  3. you should see a green background as defined in its stylesheet

Current vs. Expected behavior

The background should be green but is red.

The styles from the subpage http://localhost:3000/leakingstyle are applied to http://localhost:3000/ but they have no layout in common and should not. It seems that because they have another CSS file in common they are merged

Another page next to it that has no css file in common does not have this issue : http://localhost:3000/otherstyle

Provide environment information

Operating System:
  Platform: linux
  Arch: x64
  Version: #1 SMP Fri Jun 2 00:45:15 UTC 2023
  Available memory (MB): 17991
  Available CPU cores: 6
Binaries:
  Node: 20.11.1
  npm: 10.2.4
  Yarn: 1.22.21
  pnpm: 8.14.1
Relevant Packages:
  next: 14.2.2
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: N/A
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Not sure

Which stage(s) are affected? (Select all that apply)

next build (local), Vercel (Deployed)

Additional context

I tested multiple versions:

  • 14.1.4 : OK
  • 14.2.0-canary.27: OK
  • 14.2.0-canary.28: BROKEN
  • 14.2.0 : BROKEN
  • 14.2.1 : BROKEN
  • 14.2.2 : BROKEN
  • 14.3.0-canary.11: BROKEN

Apparently the issue has been introduced in https://github.com/vercel/next.js/releases/tag/v14.2.0-canary.28

I would bet that the issue was introduced in this PR : #63157

@onigoetz onigoetz added the bug Issue was opened via the bug report template. label Apr 19, 2024
@iamlinkus
Copy link

+1

Experiencing the exact same, super annoying. Happens only on build though.

@astateful
Copy link

+1

This issue is directly responsible for a 5-10% performance drop on our SEO optimised pages, in which the Lighthouse "eliminate render blocking resources" diagnostic suggests a savings of 1080ms by removing unused CSS, whereas with 14.1.4 it was around 250ms.

@alexanderchabo
Copy link

Same here, happens only on build. It currently blocks us from upgrading next.js version from 14.1.4 to 14.2.3

@mvdve
Copy link

mvdve commented May 2, 2024

Unfortunately running in to the same issue where my frontend styles are merged into the backend.

@iamlinkus
Copy link

This actually also happens when there’s no CSS files in common. A single global css (non module) import in one parallel route layout results in those styles being imported in other parallel routes.

@github-actions github-actions bot added the linear: next Confirmed issue that is tracked by the Next.js team. label May 13, 2024
@iamlinkus
Copy link

This is super annoying and makes it impossible to use next.js with the payloadcms 3.0, unless you have no global styles (which is rarely the case). Is there any update on this from the next team?

@samcx
Copy link
Member

samcx commented Jun 3, 2024

@onigoetz Thank you for sharing. I can confirm this is an issue we're there is different behavior happening from next dev / next dev --turbo and next build.

We are taking a look at several CSS issues at the moment and hopefully we can land several fixes at once! Will be sharing updates and testing these :repro:s as we get them.

@jricaldi
Copy link

OMG I am not the only one. I just updated to the lastest version (14.2.4). I almost pulled the hair out of my head. xD

@balatD

This comment has been minimized.

@KnotScientific

This comment has been minimized.

@samcx
Copy link
Member

samcx commented Jul 1, 2024

No updates yet, but we are indeed prioritizing this issue!

@balatD The CSS merging works as expected versions prior to v14.2.0-canary.28 (just tested on v14.2.0-canary.27).

@timhettler
Copy link

In lieu of a +1 I will just say that my project is stuck on 14.1.x because of this issue. We are using Sass + CSS Modules, with a single global css file that contains resets and utility classes (i.e. mobile-only).

@samcx
Copy link
Member

samcx commented Jul 8, 2024

Hi everyone—

Let us know if canary v15.0.0-canary.56 or higher fixes your issue—we recently shipped a bug fix that changes how we merge global css → #67373.

@onigoetz I can no longer reproduce the issue!

CleanShot 2024-07-08 at 16 17 08@2x

CleanShot 2024-07-08 at 16 16 12@2x

As described in the :pr:, the global css file is now a separate chunk (2nd screenshot).

huozhi pushed a commit that referenced this issue Jul 9, 2024
…67373)

### What?

This disallows merging of global css with styles that appear on other
pages/chunk groups.

### Why?

Before we made the assumption that all CSS is written in a way that it
only affects the elements it should really affect.

In general writing CSS in that way is recommended. In App Router styles
are only added and never removed. This means when a user uses
client-side navigations to navigate the application, styles from all
previous pages are still active on the current page. To avoid visual
artefacts one need to write CSS in a way that it only affects certain
elements. Usually this can be archived by using class names. CSS Modules
even enforce this recommendation.

Assuming that all styles are written this way allows to optimize CSS
loading as request count can be reduced when (small) styles are merged
together.

But turns out that some applications are written differently. They use
global styles that are not scoped to a class name (e. g. to `body`
directly instead) and use them in different sections of the application.
They are structured in a way that doesn't allow client-side navigations
between these sections. This should be valid too, which makes our
assumption not always holding true.

This PR changes the algorithm so we only make that assumption for CSS
Modules, but not for global CSS. While this affects the ability to
optimize, applications usually do not use too much global CSS files, so
that can be accepted.

fixes #64773
@timhettler
Copy link

@samcx Appreciate that there is progress on this issue - Will these fixes be made in the Next 14.x?

@samcx
Copy link
Member

samcx commented Jul 19, 2024

@timhettler It's been backported to 14.2.5!

Copy link
Contributor

github-actions bot commented Aug 2, 2024

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot added the locked label Aug 2, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants