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

Mismatch of names in grid-template-areas and grid-area across different css modules with --turbo #64509

Closed
Arctomachine opened this issue Apr 15, 2024 · 10 comments · Fixed by #65986
Assignees
Labels
bug Issue was opened via the bug report template. linear: turbopack Confirmed issue that is tracked by the Turbopack team. locked Turbopack Related to Turbopack with Next.js.

Comments

@Arctomachine
Copy link

Arctomachine commented Apr 15, 2024

Link to the code that reproduces this issue

https://github.com/Arctomachine/next-grid-template-areas-bug

To Reproduce

  1. Start dev server with --turbo flag
  2. Observe devtools for different names of grid areas in grid and in cards, incorrect visuals

Current vs. Expected behavior

Expected: areas have same name in grid and cards
Current: area names mismatch. Grid will append its module name to grid-template-area, but card will append its module name to grid-area. Since they are different names, browser will not put cards into grid correctly.

Example of area name in grid:
image
And in card:
image

Provide environment information

Operating System:
  Platform: win32
  Arch: x64
  Version: Windows 10 Pro
  Available memory (MB): 32720
  Available CPU cores: 12
Binaries:
  Node: 20.10.0
  npm: N/A
  Yarn: N/A
  pnpm: N/A
Relevant Packages:
  next: 14.2.1-canary.6 // Latest available version is detected (14.2.1-canary.6).
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.1.3
Next.js Config:
  output: N/A

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

Turbopack (--turbo)

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

next dev (local)

Additional context

Only happens for turbopack in dev mode. Production build is not affected

PACK-2976

@Arctomachine Arctomachine added the bug Issue was opened via the bug report template. label Apr 15, 2024
@github-actions github-actions bot added the Turbopack Related to Turbopack with Next.js. label Apr 15, 2024
@TariqSaiyad
Copy link

I'm experiencing the same issue. I've got a grid container in the header.module.scss file: header-module-scss-module__I8X9PG__Level1

and the same grid-area name in the mega-menu.module.scss file: mega--menu-module-scss-module__cuXXBW__Level1

This completely breaks the grid and layout

@kdy1 kdy1 assigned kdy1 and unassigned kdy1 Apr 16, 2024
@timneutkens timneutkens added the linear: turbopack Confirmed issue that is tracked by the Turbopack team. label Apr 18, 2024
@cpotey
Copy link

cpotey commented Apr 24, 2024

Also experiencing this, and it's a pretty big blocker.

I work in a turborepo monorepo. Decided to try turbopack on a new Next.js app I was working on, and one of my shared components uses grid-template-areas, and has become completely confused and broken. Unsure whether to attempt to refactor things, or hope for a fix.

Think I'll just exclude turbopack/--turbo for now.

@erikbrgn
Copy link

erikbrgn commented Apr 24, 2024

I'm not entirely sure this is solely due to an issue in turbopack. It might as well be due to lightningcss doing something weird under-the-hood. Fortunately, there's a work-around for those of you who want to keep using CSS modules with grid-template-areas and grid-area until this is fixed properly.

Using turbopack in a nextjs project, this "setup" has worked for me:

/* layout.module.css */

.layout {
  grid-template-areas:
    'header header header header'
    'nav main main main'
    'nav main main main'
    'footer footer footer footer;
}

.header {
  grid-area: header;
}

.nav {
  grid-area: nav;
}

.main {
  grid-area: main;
}
/* header.module.css */

.header {
  composes: header from './layout.module.css';
  */ ... */
}
/* nav.module.css */

.nav {
  composes: nav from './layout.module.css';
  */ ... */
}
/* main.module.css */

.main {
  composes: main from './layout.module.css';
  */ ... */
}

And so on...

@timneutkens
Copy link
Member

The reason these get prefixed is that we're using LightningCSS for processing CSS which is smarter with CSS Modules, it also ensures grid names don't conflict as per the documentation: https://lightningcss.dev/css-modules.html#css-grid

Webpack's CSS modules handling doesn't do this (but it probably should). That's what is causing the mismatch as you're upgrading.

My recommendation would be to restructure the grid templates to be defined in global CSS instead of CSS Modules.

@Arctomachine
Copy link
Author

Global styles are good for general layout of whole site which would be used for every page.
But they are not good for making small isolated component displayed in one place of entire site. This is what modules are intended for.

@timneutkens
Copy link
Member

CSS Modules CSS rules are intended to be scoped, this includes e.g. CSS animations by default. The only reason grid wasn't prefixed before is that it's a newer CSS standard and webpack's CSS handling didn't have an implementation for it, whereas the more modern LightningCSS compiler we use in Turbopack does support it.

In the case that you're describing it's basically leaking grid names making them "global" even though they're defined in a CSS Module. Basically previously that was a bug with the webpack handling.

@GabenGar
Copy link
Contributor

Your second explanation makes a lot more sense than the link in the lightningcss (which sounded like mumbo jumbo).
This explanation should be added to docs somewhere and add an error page just for that (if it's possible to detect from code analysis).
It's going to trip up a lot of people if it lands into webpack. It never occurred to me that grid name declarations are basically global declarations, in part because I've never used them outside of parent/children relations within a CSS module, but it does makes sense why a bundler must also generate unique grid area names out of them.

@timneutkens timneutkens self-assigned this May 20, 2024
timneutkens added a commit that referenced this issue May 21, 2024
## What?

Requires vercel/turborepo#8176 first.

Grid scoping in CSS Modules is disabled because Webpack CSS Modules
handling doesn't handle grid currently. This ensures moving from Webpack
to Turbopack doesn't have mismatching behavior around CSS grid.

Fixes #64509
Fixes #63758
Fixes PACK-2976

<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

### Adding or Updating Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md


## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->
@cpotey
Copy link

cpotey commented May 22, 2024

Looks like this will be sorted in an upcoming release?

@timneutkens thanks for sorting this, appreciated ❤️

@timneutkens
Copy link
Member

timneutkens commented May 23, 2024

Yes indeed! I ended up talking to Devon (maintainer of Lightning CSS about it) and we agreed that having a way towards compatibility with webpack's CSS modules handling would be beneficial for people migrating to Lightning CSS and for Turbopack as everyone using it is coming from webpack's CSS modules handling right now 👍

More info here: https://x.com/timneutkens/status/1791586981337604443

Overall I still believe scoping grid is the right thing to do but having compatibility is more important 🙏

Copy link
Contributor

github-actions bot commented Jun 6, 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 Jun 6, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2024
ForsakenHarmony pushed a commit that referenced this issue Aug 16, 2024
## What?

Requires vercel/turborepo#8176 first.

Grid scoping in CSS Modules is disabled because Webpack CSS Modules
handling doesn't handle grid currently. This ensures moving from Webpack
to Turbopack doesn't have mismatching behavior around CSS grid.

Fixes #64509
Fixes #63758
Fixes PACK-2976

<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

### Adding or Updating Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md


## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->
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: turbopack Confirmed issue that is tracked by the Turbopack team. locked Turbopack Related to Turbopack with Next.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants