Fix CSS not being bundled in app dir (#45787)

Currently all import CSS resources, including CSS modules, are imported lazily. This means that they can't be chunked as by definition of "lazy" they can be loaded separately.

This PR changes it to always use "eager" so if they're in the same entry, these CSS resources can be chunked together and reduce the total amount of requests. However the downside will be tree shaking, as not all modules in a chunk are used by one entry. Two entries can only share a part of it.

Since CSS modules won't have side effects this should be a good trade off.

## Bug

- [ ] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Feature

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

## Documentation / Examples

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


Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>

packages/next/src/build/webpack/loaders/next-flight-client-entry-loader.ts

@@ -22,12 +22,11 @@ export default async function transformSource(this: any): Promise<string> {
 
   const requests = modules as string[]
   const code = requests
-    // Filter out css files on the server
+    // Filter out CSS files in the SSR compilation
     .filter((request) => (isServer ? !regexCSS.test(request) : true))
-    .map((request) =>
-      regexCSS.test(request)
-        ? `(() => import(/* webpackMode: "lazy" */ ${JSON.stringify(request)}))`
-        : `import(/* webpackMode: "eager" */ ${JSON.stringify(request)})`
+    .map(
+      (request) =>
+        `import(/* webpackMode: "eager" */ ${JSON.stringify(request)})`
     )
     .join(';\n')
 

packages/next/src/server/load-components.ts

@@ -67,15 +67,18 @@ async function loadDefaultErrorComponentsImpl(distDir: string) {
   }
 }
 
-async function loadManifest<T>(manifestPath: string, attempts = 1): Promise<T> {
-  try {
-    return require(manifestPath)
-  } catch (err) {
-    if (attempts >= 3) {
-      throw err
+/**
+ * Load manifest file with retries, defaults to 3 attempts.
+ */
+async function loadManifest<T>(manifestPath: string, attempts = 3): Promise<T> {
+  while (true) {
+    try {
+      return require(manifestPath)
+    } catch (err) {
+      attempts--
+      if (attempts <= 0) throw err
+      await new Promise((resolve) => setTimeout(resolve, 100))
     }
-    await new Promise((resolve) => setTimeout(resolve, 100))
-    return loadManifest(manifestPath, attempts + 1)
   }
 }
 

test/e2e/app-dir/app-css/index.test.ts

@@ -78,28 +78,6 @@ createNextDescribe(
           const html = await next.render('/css/css-page')
           expect(html).not.toContain('/pages/_app.css')
         })
-
-        if (!isDev) {
-          it('should not include unused css modules in the page in prod', async () => {
-            const browser = await next.browser('/css/css-page/unused')
-            expect(
-              await browser.eval(
-                `[...document.styleSheets].some(({ rules }) => [...rules].some(rule => rule.selectorText.includes('this_should_not_be_included')))`
-              )
-            ).toBe(false)
-          })
-
-          it('should not include unused css modules in nested pages in prod', async () => {
-            const browser = await next.browser(
-              '/css/css-page/unused-nested/inner'
-            )
-            expect(
-              await browser.eval(
-                `[...document.styleSheets].some(({ rules }) => [...rules].some(rule => rule.selectorText.includes('this_should_not_be_included_in_inner_path')))`
-              )
-            ).toBe(false)
-          })
-        }
       })
 
       describe('client layouts', () => {
@@ -243,6 +221,16 @@ createNextDescribe(
         })
       })
 
+      describe('chunks', () => {
+        it('should bundle css resources into chunks', async () => {
+          const html = await next.render('/dashboard')
+          expect(
+            [...html.matchAll(/<link rel="stylesheet" href="[^.]+\.css"/g)]
+              .length
+          ).toBe(3)
+        })
+      })
+
       if (isDev) {
         describe('multiple entries', () => {
           it('should only inject the same style once if used by different layers', async () => {
@@ -262,7 +250,7 @@ createNextDescribe(
             // Even if it's deduped by Float, it should still only be included once in the payload.
             // There are two matches, one for the rendered <link> and one for the flight data.
             expect(
-              initialHtml.match(/duplicate-2_style_module_css\.css/g).length
+              initialHtml.match(/css-duplicate-2\/layout\.css/g).length
             ).toBe(2)
           })
 

test/e2e/app-dir/app/index.test.ts

@@ -17,6 +17,7 @@ createNextDescribe(
   },
   ({ next, isNextDev: isDev, isNextStart, isNextDeploy }) => {
     it('should encode chunk path correctly', async () => {
+      await next.fetch('/dynamic-client/first/second')
       const browser = await next.browser('/')
       const requests = []
       browser.on('request', (req) => {
@@ -381,21 +382,25 @@ createNextDescribe(
       })
 
       it('should support rewrites on client-side navigation from pages to app with existing pages path', async () => {
+        await next.fetch('/exists-but-not-routed')
         const browser = await next.browser('/link-to-rewritten-path')
 
         try {
           // Click the link.
-          await browser.elementById('link-to-rewritten-path').click()
-          await browser.waitForElementByCss('#from-dashboard')
+          await check(async () => {
+            await browser.elementById('link-to-rewritten-path').click()
+            await browser.waitForElementByCss('#from-dashboard', 5000)
 
-          // Check to see that we were rewritten and not redirected.
-          // TODO-APP: rewrite url is broken
-          // expect(await browser.url()).toBe(`${next.url}/rewritten-to-dashboard`)
+            // Check to see that we were rewritten and not redirected.
+            // TODO-APP: rewrite url is broken
+            // expect(await browser.url()).toBe(`${next.url}/rewritten-to-dashboard`)
 
-          // Check to see that the page we navigated to is in fact the dashboard.
-          expect(await browser.elementByCss('#from-dashboard').text()).toBe(
-            'hello from app/dashboard'
-          )
+            // Check to see that the page we navigated to is in fact the dashboard.
+            expect(await browser.elementByCss('#from-dashboard').text()).toBe(
+              'hello from app/dashboard'
+            )
+            return 'success'
+          }, 'success')
         } finally {
           await browser.close()
         }
@@ -653,20 +658,27 @@ createNextDescribe(
       })
 
       it('should navigate to pages dynamic route from pages page if it overlaps with an app page', async () => {
+        await next.fetch('/dynamic-pages-route-app-overlap/app-dir')
         const browser = await next.browser('/dynamic-pages-route-app-overlap')
 
         try {
           // Click the link.
-          await browser.elementById('pages-link').click()
-          expect(await browser.waitForElementByCss('#app-text').text()).toBe(
-            'hello from app/dynamic-pages-route-app-overlap/app-dir/page'
-          )
+          await check(async () => {
+            await browser.elementById('pages-link').click()
 
-          // When refreshing the browser, the app page should be rendered
-          await browser.refresh()
-          expect(await browser.waitForElementByCss('#app-text').text()).toBe(
-            'hello from app/dynamic-pages-route-app-overlap/app-dir/page'
-          )
+            expect(
+              await browser.waitForElementByCss('#app-text', 5000).text()
+            ).toBe(
+              'hello from app/dynamic-pages-route-app-overlap/app-dir/page'
+            )
+
+            // When refreshing the browser, the app page should be rendered
+            await browser.refresh()
+            expect(await browser.waitForElementByCss('#app-text').text()).toBe(
+              'hello from app/dynamic-pages-route-app-overlap/app-dir/page'
+            )
+            return 'success'
+          }, 'success')
         } finally {
           await browser.close()
         }

test/lib/next-test-utils.js

@@ -562,7 +562,7 @@ export async function check(
   console.error('TIMED OUT CHECK: ', { regex, content, lastErr })
 
   if (hardError) {
-    throw new Error('TIMED OUT: ' + regex + '\n\n' + content)
+    throw new Error('TIMED OUT: ' + regex + '\n\n' + content + '\n\n' + lastErr)
   }
   return false
 }