Skip to content

Commit

Permalink
Remove expired link resources via MutationObserver during development (
Browse files Browse the repository at this point in the history
…#48578)

Closes NEXT-684, closes #43396.

This PR implements a temporary workaround to address the issue that some
browsers are always caching CSS resources during the lifetime of a
session. We re-introduce the versioning query to the resource to avoid
that, and then use Mutation Observer to do GC manually on the client.
Once Float handles that by itself, we can probably remove this.

Note that correctly handling GC here is **required** for correctness,
not an optimization. That's why it took us a while to address this (even
this PR is still a temporary workaround). Imagine that if you have:

```css
h1 {
  color: red;
}
```

and then you changed it to:

```css
h1 {
  font-size: 300px;
}
```

During HMR, if we don't remove the old resources but only insert the new
one, both will be applied and you will still see the `<h1>` in red,
which is wrong.

Here's a recording of this PR working correctly in Firefox:


https://user-images.githubusercontent.com/3676859/233132831-b88e4d8d-aec9-48c4-9aa7-7c7a149b377d.mp4
  • Loading branch information
shuding authored Apr 19, 2023
1 parent 9f5463d commit 191acdf
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 15 deletions.
52 changes: 52 additions & 0 deletions packages/next/src/client/app-index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -296,4 +296,56 @@ export function hydrate() {
if (isError) {
reactRoot.render(reactEl)
}

// TODO-APP: Remove this logic when Float has GC built-in in development.
if (process.env.NODE_ENV !== 'production') {
const callback = (mutationList: MutationRecord[]) => {
for (const mutation of mutationList) {
if (mutation.type === 'childList') {
for (const node of mutation.addedNodes) {
if (
'tagName' in node &&
(node as HTMLLinkElement).tagName === 'LINK'
) {
const link = node as HTMLLinkElement
if (link.dataset.precedence === 'next.js') {
const href = link.getAttribute('href')
if (href) {
const [resource, version] = href.split('?v=')
if (version) {
const allLinks = document.querySelectorAll(
`link[href^="${resource}"]`
) as NodeListOf<HTMLLinkElement>
for (const otherLink of allLinks) {
if (otherLink.dataset.precedence === 'next.js') {
const otherHref = otherLink.getAttribute('href')
if (otherHref) {
const [, otherVersion] = otherHref.split('?v=')
if (!otherVersion || +otherVersion < +version) {
otherLink.remove()
const preloadLink = document.querySelector(
`link[rel="preload"][as="style"][href="${otherHref}"]`
)
if (preloadLink) {
preloadLink.remove()
}
}
}
}
}
}
}
}
}
}
}
}
}

// Create an observer instance linked to the callback function
const observer = new MutationObserver(callback)
observer.observe(document.head, {
childList: true,
})
}
}
22 changes: 14 additions & 8 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -379,12 +379,15 @@ export async function renderToHTMLOrFlight(
? cssHrefs.map((href, index) => (
<link
rel="stylesheet"
// In dev, Safari will wrongly cache the resource if you preload it:
// In dev, Safari and Firefox will cache the resource during HMR:
// - https://github.com/vercel/next.js/issues/5860
// - https://bugs.webkit.org/show_bug.cgi?id=187726
// We used to add a `?ts=` query for resources in `pages` to bypass it,
// but in this case it is fine as we don't need to preload the styles.
href={`${assetPrefix}/_next/${href}`}
// Because of this, we add a `?v=` query to bypass the cache during
// development. We need to also make sure that the number is always
// increasing.
href={`${assetPrefix}/_next/${href}${
process.env.NODE_ENV === 'development' ? `?v=${Date.now()}` : ''
}`}
// @ts-ignore
precedence={shouldPreload ? 'high' : undefined}
key={index}
Expand Down Expand Up @@ -469,12 +472,15 @@ export async function renderToHTMLOrFlight(
? stylesheets.map((href, index) => (
<link
rel="stylesheet"
// In dev, Safari will wrongly cache the resource if you preload it:
// In dev, Safari and Firefox will cache the resource during HMR:
// - https://github.com/vercel/next.js/issues/5860
// - https://bugs.webkit.org/show_bug.cgi?id=187726
// We used to add a `?ts=` query for resources in `pages` to bypass it,
// but in this case it is fine as we don't need to preload the styles.
href={`${assetPrefix}/_next/${href}`}
// Because of this, we add a `?v=` query to bypass the cache during
// development. We need to also make sure that the number is always
// increasing.
href={`${assetPrefix}/_next/${href}${
process.env.NODE_ENV === 'development' ? `?v=${Date.now()}` : ''
}`}
// `Precedence` is an opt-in signal for React to handle
// resource loading and deduplication, etc:
// https://github.com/facebook/react/pull/25060
Expand Down
13 changes: 8 additions & 5 deletions test/e2e/app-dir/app-css/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ createNextDescribe(
const html = await next.render('/loading-bug/hi')
// The link tag should be included together with loading
expect(html).toMatch(
/<link rel="stylesheet" href="(.+)\.css"\/><h2>Loading...<\/h2>/
/<link rel="stylesheet" href="(.+)\.css(\?v=\d+)?"\/><h2>Loading...<\/h2>/
)
})

Expand Down Expand Up @@ -233,8 +233,11 @@ createNextDescribe(
it('should bundle css resources into chunks', async () => {
const html = await next.render('/dashboard')
expect(
[...html.matchAll(/<link rel="stylesheet" href="[^.]+\.css"/g)]
.length
[
...html.matchAll(
/<link rel="stylesheet" href="[^.]+\.css(\?v=\d+)?"/g
),
].length
).toBe(3)
})
})
Expand Down Expand Up @@ -280,14 +283,14 @@ createNextDescribe(
const browser = await next.browser('/css/css-duplicate/a')
expect(
await browser.eval(
`[...document.styleSheets].some(({ href }) => href.endsWith('/a/page.css'))`
`[...document.styleSheets].some(({ href }) => href.includes('/a/page.css'))`
)
).toBe(true)

// Should not load the chunk from /b
expect(
await browser.eval(
`[...document.styleSheets].some(({ href }) => href.endsWith('/b/page.css'))`
`[...document.styleSheets].some(({ href }) => href.includes('/b/page.css'))`
)
).toBe(false)
})
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/app-dir/app-css/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ import { NextResponse } from 'next/server'
export async function middleware(request) {
// This middleware is used to test Suspensey CSS
if (
request.url.endsWith('_next/static/css/app/suspensey-css/slow/page.css')
request.url.includes('_next/static/css/app/suspensey-css/slow/page.css')
) {
await new Promise((resolve) => setTimeout(resolve, 150))
} else if (
request.url.endsWith('_next/static/css/app/suspensey-css/timeout/page.css')
request.url.includes('_next/static/css/app/suspensey-css/timeout/page.css')
) {
await new Promise((resolve) => setTimeout(resolve, 1000))
}
Expand Down

0 comments on commit 191acdf

Please sign in to comment.