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

Wrap CSS chunk items in a @layer #3542

Merged

Conversation

alexkirsz
Copy link
Contributor

@alexkirsz alexkirsz commented Jan 31, 2023

In Turbopack, as a consequence of our lazy compilation model, CSS chunks can contain duplicate CSS chunk items. This can cause issues with precedence. Take the following example:

Initial CSS chunk:

/* ... */

/* chunk item A */
h1 {
  font-size: 2rem;
}

/* ... */

/* other chunk item */
h1 {
  font-size: 4rem;
}

/* ... */

Dynamic CSS chunk (loaded after the first page load completes)

/* ... */

/* chunk item A */
h1 {
  font-size: 2rem;
}

/* ... */

In this example, when the page first loads, the following rule will be applied:

h1 {
  font-size: 4rem;
}

But as soon as the dynamic CSS chunk loads, the following rule will be applied instead:

h1 {
  font-size: 2rem;
}

However, from the order of rules in the initial load, we know that the former should still apply.

We can remedy this particular issue by wrapping each CSS chunk item into its own @layer (thanks @sokra for the idea!). This ensures that when a CSS chunk item is re-encountered at a later time, it is automatically de-duplicated thanks to the inherent CSS layering algorithm.

This is not an issue in Next.js as we can't have duplicated CSS chunk items.

@alexkirsz alexkirsz requested a review from a team as a code owner January 31, 2023 13:06
@vercel
Copy link

vercel bot commented Jan 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
examples-native-web 🔄 Building (Inspect) Feb 1, 2023 at 9:32AM (UTC)
turbo-site 🔄 Building (Inspect) Visit Preview Feb 1, 2023 at 9:32AM (UTC)
8 Ignored Deployments
Name Status Preview Comments Updated
examples-basic-web ⬜️ Ignored (Inspect) Feb 1, 2023 at 9:32AM (UTC)
examples-cra-web ⬜️ Ignored (Inspect) Feb 1, 2023 at 9:32AM (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) Feb 1, 2023 at 9:32AM (UTC)
examples-kitchensink-blog ⬜️ Ignored (Inspect) Feb 1, 2023 at 9:32AM (UTC)
examples-nonmonorepo ⬜️ Ignored (Inspect) Feb 1, 2023 at 9:32AM (UTC)
examples-svelte-web ⬜️ Ignored (Inspect) Feb 1, 2023 at 9:32AM (UTC)
examples-tailwind-web ⬜️ Ignored (Inspect) Feb 1, 2023 at 9:32AM (UTC)
examples-vite-web ⬜️ Ignored (Inspect) Feb 1, 2023 at 9:32AM (UTC)

write!(s, " ({layer})")?;
}
}
Ok(ModuleId::String(s).cell())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is shared with EcmascriptChunkContextVc.

@github-actions
Copy link
Contributor

Benchmark for 31b19db

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 10.02ms ± 0.07ms 9949.45µs ± 67.39µs -0.68%
bench_hmr_to_commit/Turbopack RCC/1000 modules 10.28ms ± 0.09ms 10.26ms ± 0.09ms -0.19%
bench_hmr_to_commit/Turbopack RSC/1000 modules 489.29ms ± 2.73ms 494.51ms ± 1.50ms +1.07%
bench_hmr_to_commit/Turbopack SSR/1000 modules 10.20ms ± 0.08ms 10.23ms ± 0.09ms +0.28%
bench_hmr_to_eval/Turbopack CSR/1000 modules 8952.77µs ± 62.63µs 9005.51µs ± 87.61µs +0.59%
bench_hmr_to_eval/Turbopack RCC/1000 modules 9172.23µs ± 70.23µs 9128.57µs ± 105.43µs -0.48%
bench_hmr_to_eval/Turbopack SSR/1000 modules 9078.81µs ± 74.20µs 9094.04µs ± 92.15µs +0.17%
bench_hydration/Turbopack RCC/1000 modules 3857.68ms ± 10.55ms 3880.00ms ± 12.50ms +0.58%
bench_hydration/Turbopack RSC/1000 modules 3363.29ms ± 10.67ms 3349.27ms ± 9.96ms -0.42%
bench_hydration/Turbopack SSR/1000 modules 3109.87ms ± 12.02ms 3130.71ms ± 7.75ms +0.67%
bench_startup/Turbopack CSR/1000 modules 2097.69ms ± 11.89ms 2088.89ms ± 14.79ms -0.42%
bench_startup/Turbopack RCC/1000 modules 2524.18ms ± 8.55ms 2544.25ms ± 8.16ms +0.79%
bench_startup/Turbopack RSC/1000 modules 2439.21ms ± 4.76ms 2449.28ms ± 6.68ms +0.41%
bench_startup/Turbopack SSR/1000 modules 2071.81ms ± 5.11ms 2067.23ms ± 4.44ms -0.22%

@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2023

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Rust tests
  • Rust benchmark tests

See workflow summary for details

@alexkirsz
Copy link
Contributor Author

Adding this comment from @sokra if another issue arises in the future:

Assuming the order in one chunk is correct (it's probably not, but that's something we can fix).
And assuming the order in which chunks of one chunk group are loaded is correct (it's probably not due to optimization, the optimization need to take order into account).
We can put every css chunk item in a @layer named by it's module id (we need to add module ids for CSS for that, similar to ecmascript).
Just by doing that, we magically will have correct ordering. But I think it's worth explaining why.
When using @layer the first time it will add this layer to the global order of layers. The next time you use the same layer name, it will place the css into that existing layer.
So once we load a duplicate CSS chunk item, it will place it into the existing layer, which is earlier in the order.

@alexkirsz alexkirsz force-pushed the alexkirsz/web-456-client-adds-css-modules-stylesheets-not branch from cd47b8b to 512ad61 Compare January 31, 2023 16:15
Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. We should keep in mind that this currently breaks users using @layer. We can later fix that by moving our @layer into the user layer.

@github-actions
Copy link
Contributor

Benchmark for cf4a3e2

Test Base PR % Significant %
bench_hydration/Turbopack RSC/1000 modules 3398.75ms ± 16.51ms 3469.45ms ± 7.85ms +2.08% +0.64%
bench_startup/Turbopack SSR/1000 modules 2045.81ms ± 4.30ms 2071.11ms ± 2.00ms +1.24% +0.62%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 10.05ms ± 0.07ms 10.22ms ± 0.12ms +1.70%
bench_hmr_to_commit/Turbopack RCC/1000 modules 10.12ms ± 0.08ms 10.32ms ± 0.17ms +2.02%
bench_hmr_to_commit/Turbopack RSC/1000 modules 497.36ms ± 3.29ms 499.19ms ± 3.14ms +0.37%
bench_hmr_to_commit/Turbopack SSR/1000 modules 10.17ms ± 0.05ms 10.10ms ± 0.06ms -0.73%
bench_hmr_to_eval/Turbopack CSR/1000 modules 8948.73µs ± 106.12µs 8937.72µs ± 91.92µs -0.12%
bench_hmr_to_eval/Turbopack RCC/1000 modules 9126.95µs ± 71.26µs 9172.65µs ± 115.77µs +0.50%
bench_hmr_to_eval/Turbopack SSR/1000 modules 9123.13µs ± 68.92µs 9068.73µs ± 99.86µs -0.60%
bench_hydration/Turbopack RCC/1000 modules 3859.58ms ± 20.23ms 3860.88ms ± 14.85ms +0.03%
bench_hydration/Turbopack RSC/1000 modules 3398.75ms ± 16.51ms 3469.45ms ± 7.85ms +2.08% +0.64%
bench_hydration/Turbopack SSR/1000 modules 3160.25ms ± 12.58ms 3158.29ms ± 7.52ms -0.06%
bench_startup/Turbopack CSR/1000 modules 2101.26ms ± 11.11ms 2099.63ms ± 8.58ms -0.08%
bench_startup/Turbopack RCC/1000 modules 2619.26ms ± 24.75ms 2565.04ms ± 12.19ms -2.07%
bench_startup/Turbopack RSC/1000 modules 2424.29ms ± 8.13ms 2450.81ms ± 8.42ms +1.09%
bench_startup/Turbopack SSR/1000 modules 2045.81ms ± 4.30ms 2071.11ms ± 2.00ms +1.24% +0.62%

@alexkirsz alexkirsz force-pushed the alexkirsz/web-456-client-adds-css-modules-stylesheets-not branch from 43d96bb to 4c9cfa7 Compare February 1, 2023 09:31
@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

Benchmark for a2fa9d7

Test Base PR % Significant %
bench_startup/Turbopack RSC/1000 modules 2367.68ms ± 6.06ms 2344.36ms ± 3.28ms -0.98% -0.20%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 9304.64µs ± 93.28µs 9348.53µs ± 92.16µs +0.47%
bench_hmr_to_commit/Turbopack RCC/1000 modules 9725.54µs ± 85.67µs 9660.21µs ± 84.37µs -0.67%
bench_hmr_to_commit/Turbopack RSC/1000 modules 478.12ms ± 1.20ms 477.39ms ± 1.82ms -0.15%
bench_hmr_to_commit/Turbopack SSR/1000 modules 9462.70µs ± 73.72µs 9525.59µs ± 105.58µs +0.66%
bench_hmr_to_eval/Turbopack CSR/1000 modules 8362.69µs ± 90.20µs 8222.86µs ± 71.44µs -1.67%
bench_hmr_to_eval/Turbopack RCC/1000 modules 8489.34µs ± 82.67µs 8528.08µs ± 87.79µs +0.46%
bench_hmr_to_eval/Turbopack SSR/1000 modules 8526.30µs ± 104.60µs 8642.14µs ± 76.94µs +1.36%
bench_hydration/Turbopack RCC/1000 modules 3716.76ms ± 9.25ms 3728.09ms ± 11.50ms +0.30%
bench_hydration/Turbopack RSC/1000 modules 3275.82ms ± 10.68ms 3248.52ms ± 10.15ms -0.83%
bench_hydration/Turbopack SSR/1000 modules 3029.78ms ± 12.38ms 3033.35ms ± 8.39ms +0.12%
bench_startup/Turbopack CSR/1000 modules 2025.55ms ± 10.46ms 2048.78ms ± 10.99ms +1.15%
bench_startup/Turbopack RCC/1000 modules 2460.05ms ± 6.63ms 2468.98ms ± 4.89ms +0.36%
bench_startup/Turbopack RSC/1000 modules 2367.68ms ± 6.06ms 2344.36ms ± 3.28ms -0.98% -0.20%
bench_startup/Turbopack SSR/1000 modules 2002.67ms ± 4.81ms 1992.62ms ± 2.67ms -0.50%

@alexkirsz alexkirsz merged commit 8632812 into main Feb 1, 2023
@alexkirsz alexkirsz deleted the alexkirsz/web-456-client-adds-css-modules-stylesheets-not branch February 1, 2023 12:58
mehulkar pushed a commit that referenced this pull request Feb 1, 2023
In Turbopack, as a consequence of our lazy compilation model, CSS chunks
can contain duplicate CSS chunk items. This can cause issues with
precedence. Take the following example:

Initial CSS chunk:
```css
/* ... */

/* chunk item A */
h1 {
  font-size: 2rem;
}

/* ... */

/* other chunk item */
h1 {
  font-size: 4rem;
}

/* ... */
```

Dynamic CSS chunk (loaded after the first page load completes)
```css
/* ... */

/* chunk item A */
h1 {
  font-size: 2rem;
}

/* ... */
```

In this example, when the page first loads, the following rule will be
applied:
```css
h1 {
  font-size: 4rem;
}
```

But as soon as the dynamic CSS chunk loads, the following rule will be
applied instead:
```css
h1 {
  font-size: 2rem;
}
```

However, from the order of rules in the initial load, we know that the
former should still apply.

We can remedy this particular issue by wrapping each CSS chunk item into
its own
[`@layer`](https://developer.mozilla.org/en-US/docs/Web/CSS/@layer)
(thanks @sokra for the idea!). This ensures that when a CSS chunk item
is re-encountered at a later time, it is automatically de-duplicated
thanks to the inherent CSS layering algorithm.

This is not an issue in Next.js as we can't have duplicated CSS chunk
items.
jridgewell pushed a commit to vercel/next.js that referenced this pull request Mar 10, 2023
In Turbopack, as a consequence of our lazy compilation model, CSS chunks
can contain duplicate CSS chunk items. This can cause issues with
precedence. Take the following example:

Initial CSS chunk:
```css
/* ... */

/* chunk item A */
h1 {
  font-size: 2rem;
}

/* ... */

/* other chunk item */
h1 {
  font-size: 4rem;
}

/* ... */
```

Dynamic CSS chunk (loaded after the first page load completes)
```css
/* ... */

/* chunk item A */
h1 {
  font-size: 2rem;
}

/* ... */
```

In this example, when the page first loads, the following rule will be
applied:
```css
h1 {
  font-size: 4rem;
}
```

But as soon as the dynamic CSS chunk loads, the following rule will be
applied instead:
```css
h1 {
  font-size: 2rem;
}
```

However, from the order of rules in the initial load, we know that the
former should still apply.

We can remedy this particular issue by wrapping each CSS chunk item into
its own
[`@layer`](https://developer.mozilla.org/en-US/docs/Web/CSS/@layer)
(thanks @sokra for the idea!). This ensures that when a CSS chunk item
is re-encountered at a later time, it is automatically de-duplicated
thanks to the inherent CSS layering algorithm.

This is not an issue in Next.js as we can't have duplicated CSS chunk
items.
sokra pushed a commit to vercel/next.js that referenced this pull request Mar 13, 2023
In Turbopack, as a consequence of our lazy compilation model, CSS chunks
can contain duplicate CSS chunk items. This can cause issues with
precedence. Take the following example:

Initial CSS chunk:
```css
/* ... */

/* chunk item A */
h1 {
  font-size: 2rem;
}

/* ... */

/* other chunk item */
h1 {
  font-size: 4rem;
}

/* ... */
```

Dynamic CSS chunk (loaded after the first page load completes)
```css
/* ... */

/* chunk item A */
h1 {
  font-size: 2rem;
}

/* ... */
```

In this example, when the page first loads, the following rule will be
applied:
```css
h1 {
  font-size: 4rem;
}
```

But as soon as the dynamic CSS chunk loads, the following rule will be
applied instead:
```css
h1 {
  font-size: 2rem;
}
```

However, from the order of rules in the initial load, we know that the
former should still apply.

We can remedy this particular issue by wrapping each CSS chunk item into
its own
[`@layer`](https://developer.mozilla.org/en-US/docs/Web/CSS/@layer)
(thanks @sokra for the idea!). This ensures that when a CSS chunk item
is re-encountered at a later time, it is automatically de-duplicated
thanks to the inherent CSS layering algorithm.

This is not an issue in Next.js as we can't have duplicated CSS chunk
items.
alexkirsz added a commit that referenced this pull request Mar 14, 2023
@alexkirsz alexkirsz mentioned this pull request Mar 14, 2023
kodiakhq bot pushed a commit that referenced this pull request Mar 14, 2023
### Description

The fix introduced in #3542 was a workaround for CSS in dynamic imports
being included multiple times in a page, potentially overriding
precedence of earlier chunks, but introduced its own precedence issue
(the semantics of `@layer` are not fully compatible with our use case).
This is no longer necessary since #4056 made it so dynamic imports no
longer include items from the parent chunk.

### Testing Instructions

create-next-app with app support now looks visually identical with and
without --turbo.

Snapshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants