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

Fix CSS modules imported from client components in app dir with next build #38329

Merged
merged 4 commits into from
Jul 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ function generateClientManifest(
})
}

function getEntrypointFiles(entrypoint: any): string[] {
export function getEntrypointFiles(entrypoint: any): string[] {
return (
entrypoint
?.getFiles()
Expand Down
32 changes: 24 additions & 8 deletions packages/next/build/webpack/plugins/flight-manifest-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { webpack, sources } from 'next/dist/compiled/webpack/webpack'
import { FLIGHT_MANIFEST } from '../../../shared/lib/constants'
import { clientComponentRegex } from '../loaders/utils'
import { relative } from 'path'
import { getEntrypointFiles } from './build-manifest-plugin'

// This is the module that will be used to anchor all client references to.
// I.e. it will have all the client files as async deps from this point on.
Expand Down Expand Up @@ -129,20 +130,35 @@ export class FlightManifestPlugin {
)
.filter((name) => name !== null)

// Get all CSS files imported in that chunk.
const cssChunks: string[] = []
for (const entrypoint of chunk._groups) {
if (entrypoint.getFiles) {
const files = getEntrypointFiles(entrypoint)
for (const file of files) {
if (file.endsWith('.css')) {
cssChunks.push(file)
}
}
}
}

moduleExportedKeys.forEach((name) => {
if (!moduleExports[name]) {
moduleExports[name] = {
id,
name,
chunks: appDir
? chunk.ids.map((chunkId: string) => {
return (
chunkId +
':' +
(chunk.name || chunkId) +
(dev ? '' : '-' + chunk.hash)
)
})
? chunk.ids
.map((chunkId: string) => {
return (
chunkId +
':' +
(chunk.name || chunkId) +
(dev ? '' : '-' + chunk.hash)
)
})
.concat(cssChunks)
: [],
}
}
Expand Down
12 changes: 12 additions & 0 deletions packages/next/client/app-next.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { hydrate, version } from './app-index'

// Include app-router and layout-router in the main chunk
import 'next/dist/client/components/app-router.client.js'
import 'next/dist/client/components/layout-router.client.js'

window.next = {
version,
root: true,
Expand All @@ -23,6 +27,14 @@ self.__next_require__ = __webpack_require__

// eslint-disable-next-line no-undef
self.__next_chunk_load__ = (chunk) => {
if (chunk.endsWith('.css')) {
const link = document.createElement('link')
link.rel = 'stylesheet'
link.href = '/_next/' + chunk
document.head.appendChild(link)
return Promise.resolve()
Comment on lines +31 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach isn't going to work with React Float. it might make sense to use float here (when ready) to call preload but we shouldn't insert the link element into the dom this way.

I thought that the app router and layout router got passed the manifest. The router itself should do the link insertion using regular rendering of react elements.

Is that just intended for a later implementation?

}

const [chunkId, chunkFileName] = chunk.split(':')
chunkFilenameMap[chunkId] = `static/chunks/${chunkFileName}.js`

Expand Down
37 changes: 34 additions & 3 deletions packages/next/server/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ function useFlightResponse(
rscCache.set(id, entry)

let bootstrapped = false
let remainingFlightResponse = ''
const forwardReader = forwardStream.getReader()
const writer = writable.getWriter()
function process() {
Expand All @@ -138,11 +139,41 @@ function useFlightResponse(
rscCache.delete(id)
writer.close()
} else {
const responsePartial = decodeText(value)
const css = responsePartial
.split('\n')
.map((partialLine) => {
const line = remainingFlightResponse + partialLine
remainingFlightResponse = ''

try {
const match = line.match(/^M\d+:(.+)/)
if (match) {
return JSON.parse(match[1])
.chunks.filter((chunkId: string) =>
chunkId.endsWith('.css')
)
.map(
(file: string) =>
`<link rel="stylesheet" href="/_next/${file}">`
)
.join('')
}
return ''
} catch (err) {
// The JSON is partial
remainingFlightResponse = line
return ''
}
Comment on lines +163 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than overparsing why not rewrite this a while loop that looks for the next line break. only process if there is a next line break.

Here is basically the same algo in React: https://github.com/facebook/react/blob/main/packages/react-client/src/ReactFlightClientStream.js#L100-L109

})
.join('')

writer.write(
encodeText(
`<script>(self.__next_s=self.__next_s||[]).push(${htmlEscapeJsonString(
JSON.stringify([1, id, decodeText(value)])
)})</script>`
css +
`<script>(self.__next_s=self.__next_s||[]).push(${htmlEscapeJsonString(
JSON.stringify([1, id, responsePartial])
)})</script>`
)
)
process()
Expand Down
22 changes: 9 additions & 13 deletions test/e2e/app-dir/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import path from 'path'
import cheerio from 'cheerio'
import webdriver from 'next-webdriver'

const isDev = (global as any).isNextDev

describe('views dir', () => {
if ((global as any).isNextDeploy) {
it('should skip next deploy for now', () => {})
Expand Down Expand Up @@ -289,17 +287,15 @@ describe('views dir', () => {
})

describe('css support', () => {
if (isDev) {
it('should support css modules inside client layouts', async () => {
const browser = await webdriver(next.url, '/client-nested')
it('should support css modules inside client layouts', async () => {
const browser = await webdriver(next.url, '/client-nested')

// Should render h1 in red
expect(
await browser.eval(
`window.getComputedStyle(document.querySelector('h1')).color`
)
).toBe('rgb(255, 0, 0)')
})
}
// Should render h1 in red
expect(
await browser.eval(
`window.getComputedStyle(document.querySelector('h1')).color`
)
).toBe('rgb(255, 0, 0)')
})
})
})