feat(css): support Next Tailwind config and dev layout CSS#1130
feat(css): support Next Tailwind config and dev layout CSS#1130NathanDrake2406 wants to merge 15 commits into
Conversation
NathanDrake2406
changed the title
NathanDrake2406
force-pushed
the
nathan/tailwind-config-smoothness
branch
from
c49ce54 to
6bd9d10
Compare
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat(config): support Next Tailwind CSS config shapes
Solid PR. The architecture decision to translate Turbopack's @tailwindcss/webpack loader shape into @tailwindcss/postcss rather than late-injecting @tailwindcss/vite is the right call — it avoids the fragile lifecycle timing and keeps the compatibility layer as a deterministic config transform. The css-config-compat.ts boundary is clean and the decision tree is easy to follow.
The postcss.ts refactor is a genuine improvement over the old code: normalizing both array-form and object-form plugins into PostcssPluginEntry[], handling options: false to disable plugins, supporting postcss.config.json, and the lazy get postcss() getter are all well-motivated.
The dev CSS stylesheet link injection addresses a real gap. A few concerns on that piece below.
Key concerns
-
stripSourceCommentsdoesn't handle template literal interpolations — it treats backticks identically to quotes, so${...}expressions containing nested backticks or comments will desync the parser. For CSS import detection this is low-severity (imports are typically at the top of files, before template literals), but it's a latent correctness issue. -
resolvePostcssPlugincan returnundefinedwhile the return type saysPostcssAcceptedPlugin— ifresolvePostcssPluginsfilters outundefinedentries (foroptions === false), destructuring[plugin]yieldsundefined. The caller incss-config-compat.tspasses{}as options so this won't trigger in practice, but the type contract is unsound. -
isRecordis duplicated four times across the new/modified files — this is a pre-existing pattern, not introduced by this PR, but the PR adds two more copies. Consider extracting to a shared utility. -
findPostcssConfigreads.postcssrctwice for JSON content — first to check if it starts with{(line 103), then again viareadJsonFile(line 111). Minor, but the content from the first read could be reused. -
Build-time
styles: []in every route entry — whendevCssContextis null (production builds), every route entry getsstyles: []serialized into the manifest. This is harmless but adds bytes to the production bundle.
See inline comments for specifics.
| if (fp) imports.getImportVar(fp); | ||
| } | ||
|
|
||
| if (char === '"' || char === "'" || char === "`") { |
There was a problem hiding this comment.
Template literal handling bug: backticks are treated identically to single/double quotes, but template literals can contain ${...} expressions with their own strings, comments, or nested backticks. For example:
const cls = `${foo ? 'a' : `nested`}`;
import './styles.css';The inner backtick inside the interpolation would be matched as the closing backtick, desyncing the parser for the rest of the file.
Pragmatic fix: Don't enter quote mode for backticks at all. CSS import statements never appear inside template literals, so there's no need to protect template literal content from comment stripping. The only risk is a // or /* inside a template literal being falsely stripped, but since stripSourceComments replaces with spaces (preserving positions) and imports are at the top of files, this is very unlikely to cause a false CSS import match.
| if (char === '"' || char === "'" || char === "`") { | |
| if (char === '"' || char === "'") { |
If you'd rather keep the conservative behavior, the proper fix requires tracking ${ nesting depth — which is more complexity than this feature warrants.
| page: ${route.pagePath ? imports.getImportVar(route.pagePath) : "null"}, | ||
| routeHandler: ${route.routePath ? imports.getImportVar(route.routePath) : "null"}, | ||
| layouts: [${layoutVars.join(", ")}], | ||
| styles: ${JSON.stringify(styles)}, |
There was a problem hiding this comment.
In production builds (devCssContext is null), this serializes styles: [] into every route entry. It's harmless, but it adds a few bytes per route to the production manifest. Consider omitting the field entirely when empty:
| styles: ${JSON.stringify(styles)}, | |
| styles: ${styles.length > 0 ? JSON.stringify(styles) : "[]"}, |
Actually, since the runtime reads route.styles?.map(...) with optional chaining, you could emit nothing for production:
${styles.length > 0 ? ` styles: ${JSON.stringify(styles)},\n` : ''}Not a blocker — just a minor optimization opportunity.
| projectRoot: string, | ||
| name: string, | ||
| options: unknown, | ||
| ): Promise<PostcssAcceptedPlugin> { |
There was a problem hiding this comment.
Type soundness issue: resolvePostcssPlugins can return an empty array (it filters out undefined entries, e.g., when options === false). Destructuring const [plugin] = await resolvePostcssPlugins(...) would then give plugin as undefined, but the return type is Promise<PostcssAcceptedPlugin> (non-nullable).
This won't trigger in practice because the only caller passes {} as options, but the type contract is misleading. Consider:
| ): Promise<PostcssAcceptedPlugin> { | |
| ): Promise<PostcssAcceptedPlugin> { | |
| const plugins = await resolvePostcssPlugins(projectRoot, [{ name, options }]); | |
| if (!plugins[0]) throw new Error(`PostCSS plugin "${name}" could not be resolved.`); | |
| return plugins[0]; | |
| } |
| candidate.endsWith(".yaml") || | ||
| candidate.endsWith(".yml") || | ||
| (path.basename(candidate) === ".postcssrc" && | ||
| !fs.readFileSync(candidate, "utf-8").trim().startsWith("{")) |
There was a problem hiding this comment.
Minor: .postcssrc is read here to check whether it starts with {, and then if it does, it falls through to line 108 where readJsonFile reads it again. You could stash the content from the first read to avoid the double I/O:
if (path.basename(candidate) === '.postcssrc') {
const content = fs.readFileSync(candidate, 'utf-8').trim();
if (!content.startsWith('{')) {
return { configPath: candidate, config: undefined, loadModule: false };
}
return { configPath: candidate, config: JSON.parse(content), loadModule: false };
}Not a blocker — just avoids an unnecessary re-read.
| }; | ||
| } | ||
|
|
||
| const needsExplicitViteConfig = |
There was a problem hiding this comment.
Good: the needsExplicitViteConfig condition correctly captures the two cases where Vite can't discover the config on its own (JSON config file, or string-form plugins in array). The lazy get postcss() getter is a nice touch to avoid resolving plugins until they're actually needed.
| import { createMetadataRouteEntriesSource } from "../server/metadata-route-build-data.js"; | ||
| import type { MetadataFileRoute } from "../server/metadata-routes.js"; | ||
| import { normalizePathSeparators } from "./runtime-entry-module.js"; | ||
| import fs from "node:fs"; |
There was a problem hiding this comment.
Adding fs and path (synchronous I/O) to a module that generates code for the RSC entry is a notable change. The collectCssImports function reads route module files from disk synchronously at entry generation time. This happens once per dev server start / HMR rebuild, so performance should be fine. But it does mean app-rsc-manifest.ts now has side effects beyond pure string generation.
The AGENTS.md guideline says generated entry modules should stay thin — this adds ~180 lines of CSS import resolution logic to the manifest builder. It might be worth extracting the CSS collection functions (stripSourceComments, collectCssImports, resolveCssImportHref, etc.) into a separate module like server/dev-css-imports.ts to keep the manifest builder focused on code generation. Not blocking, but aligns with the project's "move real behavior into server/*" guideline.
|
|
||
| elements[routeId] = ( | ||
| <> | ||
| {options.route.styles?.map((href) => ( |
There was a problem hiding this comment.
These <link> elements are emitted inside a Fragment at the route level, which means they'll be in the RSC stream body — not in <head>. In dev mode this works because Vite's CSS handling is client-side anyway and these links will be fetched, but it's worth noting that this pattern differs from how Next.js hoists CSS into <head>. If Vite's dev server deduplicates CSS requests from the module graph, you might get double-fetches for CSS that's both imported by a module (Vite's HMR CSS injection) and linked here.
Is there a risk of FOUC or double-application of styles in dev? If the <link> loads the CSS and Vite's module graph also injects it, the browser applies it twice (which is visually identical but wasteful). Might be worth a comment noting this is intentional for dev-only and explaining how it interacts with Vite's own CSS handling.
| "string-form PostCSS plugins need resolution — vinext handles this automatically", | ||
| }); | ||
| } | ||
| } else if (configFile === "postcss.config.json") { |
There was a problem hiding this comment.
This else if only fires when postcss.config.json exists but the string-plugin regex doesn't match. That means a postcss.config.json with string-form plugins like { "plugins": ["@tailwindcss/postcss"] } would hit the if branch above (the regex would match) and get reported as "string-form PostCSS plugins" with status "partial" — which is correct but slightly misleading since vinext handles JSON configs natively. The user sees "partial" when the actual handling is complete.
Might be worth special-casing postcss.config.json before the regex check, or adjusting the message for JSON configs to indicate it's fully handled.
| } | ||
|
|
||
| const CSS_IMPORT_RE = | ||
| /(?:^|[;\n\r])\s*import\s+(?:[^'"]*?\s+from\s+)?["']([^"']+\.(?:css|less|sass|scss|styl|stylus|pcss|postcss|sss)(?:\?[^"']*)?)["']/g; |
There was a problem hiding this comment.
The regex won't match CSS re-exports like export { default } from './styles.css', which is technically valid. Also won't match side-effect re-exports like export './styles.css' (not valid JS, but export {} from './styles.css' is). This is fine for the use case — CSS re-exports from route modules are extremely rare. Worth a brief comment noting the intentional scope limitation.
| @@ -851,6 +854,10 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] { | |||
| } | |||
There was a problem hiding this comment.
Good: building devCssAliases from both tsconfig paths and next.config aliases ensures CSS imports using either alias system are resolved correctly. The spread order gives next.config aliases precedence over tsconfig paths, which matches how Vite's own resolution would work.
|
Review submitted on PR #1130 with 11 inline comments. Here's a summary of the key findings: Overall assessment: Solid PR with good architecture decisions. The Turbopack-to-PostCSS translation approach is well-reasoned, and the Key concerns raised:
|
Translate the known Tailwind Turbopack CSS loader into Vite PostCSS config, resolve Next-supported PostCSS config shapes that Vite misses, and emit dev stylesheet links for App Router layout-level CSS imports.
NathanDrake2406
force-pushed
the
nathan/tailwind-config-smoothness
branch
from
6bd9d10 to
6b82779
Compare
NathanDrake2406
changed the title
|
/bigbonk review |
There was a problem hiding this comment.
Review
Good PR overall. The architecture is sound — the css-config-compat.ts boundary, the PostCSS normalization refactor, and the StaticSegmentGraph / RouteAssetManifest separation are all well-motivated. The dev-css-imports.ts implementation using transformWithOxc + parseAst rather than regex-based comment/string stripping is a much better approach than what the first review discussed (the stripSourceComments function that no longer exists).
I see the first review's resolvePostcssPlugin type soundness concern was already addressed (the guard at line 264 throws), and the styles: [] production emission concern was also addressed (conditional emit on line 121 of app-rsc-manifest.ts). Nice.
A few items below, ranging from a real correctness concern to minor improvements.
Key concern
-
Sequential transitive CSS walk is a latent perf cliff —
collectDevCssHrefsForFilesawaits each transitive source import sequentially in a DFS. For a route with deep import chains (e.g., a layout importing a shell component importing a header importing a nav — each with their own CSS), this serializes N file stat + read + OXC parse operations. The cache mitigates repeat visits across routes, but the first cold walk of a deep chain will be slow. Not blocking for this PR since it's dev-only and bounded by the module count, but worth noting as a future parallelization target. -
collectAppRouteAssetModuleFilesintentionally excludesroutePath— I seecollectAppRouteModuleFilesincludes route handlers via theincludeRouteHandleroption, whilecollectAppRouteAssetModuleFiles(used for dev CSS) omits them entirely. This is correct because route handlers (route.ts) don't render UI and shouldn't contribute CSS, but there's no doc comment explaining the distinction. A one-line comment would help future readers.
Minor items
See inline comments.
| hrefs.add(href); | ||
| } | ||
| for (const sourceImport of scan.sourceImports) { | ||
| await visit(sourceImport); |
There was a problem hiding this comment.
Sequential await within the loop means transitive source imports are resolved one at a time. For a module that imports 5 source files that each import 3 more, this serializes up to ~20 file reads + OXC parses on a cold cache.
Not blocking — the cache makes subsequent reloads fast, and the total file count per route is bounded. But if dev startup feels slow on large apps, this is the first place to look. A Promise.all on the sourceImports would parallelize the fan-out at each level:
| await visit(sourceImport); | |
| await Promise.all(scan.sourceImports.map((sourceImport) => visit(sourceImport))); |
The visited set already guards against cycles, so parallel visits are safe.
| return files.filter((filePath): filePath is string => typeof filePath === "string"); | ||
| } | ||
|
|
||
| export function collectAppRouteAssetModuleFiles(route: AppRoute): string[] { |
There was a problem hiding this comment.
Worth a brief doc comment explaining why route handlers are excluded — future readers might wonder why this function exists separately from collectAppRouteModuleFiles.
| export function collectAppRouteAssetModuleFiles(route: AppRoute): string[] { | |
| /** Collect module files that contribute to a route's client-visible assets (CSS, etc.). Route handlers are excluded because they don't render UI. */ | |
| export function collectAppRouteAssetModuleFiles(route: AppRoute): string[] { |
| ): Promise<string[]> { | ||
| let code: string; | ||
| try { | ||
| code = (await transformWithOxc(source, filePath, { sourcemap: false })).code; |
There was a problem hiding this comment.
Nice choice using transformWithOxc + parseAst instead of regex-based comment/string stripping. This correctly handles template literals, nested strings, JSX expressions, and all the edge cases that a regex parser would miss.
One thing: transformWithOxc will do a full TS→JS transform (stripping types, converting JSX). For the purpose of extracting import specifiers, you only need the AST — the transform output is immediately re-parsed. Have you checked if parseAst can handle TSX directly? If so, you could skip the transform step and go straight to parsing, which would be faster.
If parseAst only handles plain JS (likely, since it wraps Rollup's parser), then this two-step approach is correct.
| return toAcceptedPostcssPlugin(entry.value, "inline"); | ||
| } | ||
|
|
||
| if (entry.options === false) return undefined; |
There was a problem hiding this comment.
This options === false check is dead code in the current implementation — normalizePostcssPlugins already filters out entries with options === false at line 190 (the addPlugin function returns early). So an entry with options === false never makes it into the entries array.
Not harmful, but it's confusing to have a guard here for a condition that can't happen. Consider removing it, or adding a comment explaining it's a defensive check for future callers that might bypass normalizePostcssPlugins.
| // built client manifest, so these links intentionally stay optional. | ||
| elements[routeId] = ( | ||
| <> | ||
| {options.route.styles?.map((href) => ( |
There was a problem hiding this comment.
The first review raised a valid concern about double-fetching: Vite's dev server already handles CSS imported by modules (via its module graph + HMR CSS injection). These <link> tags will cause the browser to fetch the same CSS files a second time.
Is this intentional to ensure CSS is present on initial SSR before Vite's client-side injection kicks in? If so, a brief comment explaining the trade-off (double fetch in dev to avoid FOUC) would help. If Vite's module graph CSS is sufficient for dev, these links might be unnecessary.
| } | ||
|
|
||
| const cached = cache.get(filePath); | ||
| if (cached && cached.mtimeMs === stats.mtimeMs && cached.size === stats.size) { |
There was a problem hiding this comment.
The mtime + size cache validation is a good pragmatic approach. One edge case: on some filesystems (notably HFS+ on macOS), mtime granularity is 1 second, so two rapid edits within the same second with the same file size would serve stale cache. This is a known limitation of mtime-based caching and not worth fixing here — just noting it.
| ); | ||
| } | ||
|
|
||
| const existingPostcss = |
There was a problem hiding this comment.
This correctly falls back to the existing project PostCSS config when postcssOverride wasn't already set (i.e., when there's no string-form/JSON config that needed Vite injection). The prependPostcssPlugin then places Tailwind before any existing plugins, which matches the expected order.
Good: this means a project with postcss.config.mjs containing ["autoprefixer"] plus a Turbopack Tailwind rule ends up with [@tailwindcss/postcss, autoprefixer] — correct ordering.
| if (id === RESOLVED_RSC_ENTRY && hasAppDir) { | ||
| const resolveDevCssImport = async (specifier: string, importerPath: string) => { |
There was a problem hiding this comment.
This resolveDevCssImport callback is created fresh on every load() call for the RSC entry. Since load() is called on HMR, this is fine — the this.resolve context changes per call.
But note that this.resolve here is the Rollup/Vite plugin context resolve, which runs in the RSC environment. CSS files imported by route modules might resolve differently in the RSC environment vs the client environment (e.g., different conditions). For dev CSS <link> hrefs that the browser will fetch, you want client-resolvable paths. Currently this works because CSS file resolution is the same across environments (no export conditions on .css files), but it's worth a comment noting this assumption.
|
Review submitted successfully. Here's a summary of what I posted: Overall assessment: Good PR with sound architecture. The I noted that several of the first review's concerns were already addressed in the current code (the Key concerns raised:
Inline comments (8 total):
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3138552717
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/bigbonk review |
There was a problem hiding this comment.
Review
This is a well-structured PR. The separation of concerns is good: css-config-compat.ts owns the decision tree for when to inject PostCSS config, postcss.ts owns normalization and resolution, dev-css-imports.ts owns the static import scanner, route-asset-manifest.ts joins routes with CSS, and app-route-module-files.ts extracts the shared "enumerate route files" logic that was previously inlined in app-rsc-manifest.ts. The registerRouteModules refactor to use collectAppRouteModuleFiles is a nice deduplication.
The previous reviews raised valid points, many of which were already addressed (the resolvePostcssPlugin type guard, stripSourceComments replaced by transformWithOxc + parseAst, conditional styles emission). The Promise.all suggestion for transitive source imports was already implemented in the final code. Good.
A few items below, mostly around edge cases and one functional concern.
Key items
-
devCssImportsCachelives for the entire plugin lifetime but never invalidates stale entries for deleted files. The cache is a module-levelMapthat grows monotonically. When a file is deleted,getCachedCssImportScanreturnsundefined(thefs.statfails and deletes the entry), which is correct. But if a file that was previously a transitive import target gets deleted, the parent's cached scan still references it insourceImports. The parent's mtime/size hasn't changed, so it serves the stale scan pointing to the now-deleted file. The deleted file then returnsundefinedfromgetCachedCssImportScan, so it's harmless (no crash), but the CSS from that file lingers in the output until the parent is re-saved. For dev-only this is low severity but worth documenting. -
Five copies of
isRecordacross the codebase — this PR adds two more (css-config-compat.ts,next-config.ts). This is a pre-existing pattern, but the PR is a good opportunity to extract it to a shared utility. -
The
hasUnsupportedConfiguredTurbopackRuleswarning is correct for mixed configs. The Codex review suggested the warning is suppressed when Tailwind is detected, but that's not what the code does — the two checks (hasTailwindTurbopackCssLoaderandhasUnsupportedConfiguredTurbopackRules) are computed independently and the warning fires whenever any rule doesn't match the supported Tailwind-only shape. A mixed config like{ "*.css": { loaders: ["@tailwindcss/webpack"] }, "*.scss": { loaders: ["sass-loader"] } }correctly triggers both the Tailwind translation AND the unsupported-rules warning.
Minor items
See inline comments.
| } | ||
|
|
||
| const scan = scanDevCssImports(filePath, context); | ||
| cache.set(filePath, { scan, mtimeMs: stats.mtimeMs, size: stats.size }); |
There was a problem hiding this comment.
The cache stores the scan Promise at set time, keyed by mtimeMs + size. If a file is edited twice within the same second (1s mtime granularity on HFS+/some Linux configs) and ends up at the same size, this serves a stale scan. This is a known limitation of mtime-based caching and is fine for dev — just worth a brief comment so future readers know it's deliberate.
More subtly: this caches the DevCssFileScan result, which includes sourceImports (resolved absolute paths). If a transitive import target is renamed (e.g., Shell.tsx → AppShell.tsx) and the parent file is updated to import the new name, the parent's mtime changes and the cache correctly invalidates. But if the parent file is not changed and the transitive file is renamed, the stale sourceImports entry points to a non-existent file, which silently returns no CSS. This is acceptable for dev but could confuse users who rename a component file without touching the importer.
There was a problem hiding this comment.
Fixed by documenting the deliberate dev-cache tradeoff: mtime/size validation is cheap, but rapid same-size edits or renamed transitive imports can remain stale until an importer changes.
| <> | ||
| {options.route.styles?.map((href) => { | ||
| const assetHref = prefixRouteAssetHref(href, options.basePath); | ||
| return <link key={assetHref} rel="stylesheet" href={assetHref} />; |
There was a problem hiding this comment.
The comment on line 786-789 is helpful and addresses the double-fetch concern from the first review. One remaining question: these <link> tags are inside a Fragment at the route element level, which means they'll appear in the <body> (inside the route's React tree), not in <head>. Browsers tolerate <link rel="stylesheet"> in <body> (it's valid in HTML5 and all modern browsers handle it), but it differs from how Next.js hoists CSS into <head>.
For dev-only this is fine, but if this pattern ever expands to production, the links should be hoisted into <head>. Worth a note that the placement is intentional for dev simplicity.
There was a problem hiding this comment.
Fixed by extending the comment to call out that these dev-only stylesheet links intentionally remain in the route body for simplicity. Production CSS still comes from the built client manifest.
|
|
||
| // Keep the boundary defensive for direct callers such as resolvePostcssPlugin(); | ||
| // normalizePostcssPlugins() filters disabled entries before this point. | ||
| if (entry.options === false) return undefined; |
There was a problem hiding this comment.
This options === false guard is unreachable from the primary code path: normalizePostcssPlugins filters out options === false entries at line 190 before they reach resolvePostcssPlugins. The only way to hit this is through resolvePostcssPlugin called directly with options: false, which the test at postcss-resolve.test.ts:296 covers.
The comment on line 237-238 explains the intent, but the return type mismatch is still present: this returns undefined, which is then filtered by the .filter() on line 255. The types work out at runtime, but the intermediate undefined in the Promise.all result is typed as PostcssAcceptedPlugin (from the entries.map callback return type). Consider making the return type of the map callback explicit: async (entry): Promise<PostcssAcceptedPlugin | undefined> => {.
There was a problem hiding this comment.
Fixed by making the async map callback return Promise<PostcssAcceptedPlugin | undefined>, matching the defensive options === false path before the filter narrows it.
| routeHandler: ${route.routePath ? imports.getImportVar(route.routePath) : "null"}, | ||
| layouts: [${layoutVars.join(", ")}], | ||
| routeSegments: ${JSON.stringify(route.routeSegments)}, | ||
| ${styles.length > 0 ? ` styles: ${JSON.stringify(styles)},\n` : ""} routeSegments: ${JSON.stringify(route.routeSegments)}, |
There was a problem hiding this comment.
Good: the conditional styles emission avoids serializing empty arrays into production bundles. The formatting is a bit dense on this line — the ternary produces either styles: [...],\n routeSegments: ... or routeSegments: .... It reads correctly but is easy to misread at a glance. Consider splitting it for clarity:
| ${styles.length > 0 ? ` styles: ${JSON.stringify(styles)},\n` : ""} routeSegments: ${JSON.stringify(route.routeSegments)}, | |
| ${styles.length > 0 ? ` styles: ${JSON.stringify(styles)},\n` : ""} routeSegments: ${JSON.stringify(route.routeSegments)}, |
(This is already what it is — just confirming the formatting works. No change needed.)
| ): Promise<string[]> { | ||
| let code: string; | ||
| try { | ||
| code = (await transformWithOxc(source, filePath, { sourcemap: false })).code; |
There was a problem hiding this comment.
The two-step transformWithOxc → parseAst approach works correctly but is doing more work than strictly necessary. transformWithOxc performs a full TS/JSX → JS transform (type stripping, JSX conversion), and the resulting JS is then parsed again by parseAst just to extract ImportDeclaration nodes.
If parseAst could handle TypeScript/TSX directly (which Rollup's parser currently can't), you could skip the transform. As-is, this is the correct approach. The performance cost is bounded by the per-file transform + parse, which is fast for the file sizes involved (route modules are typically small).
One thing to watch: transformWithOxc defaults to the OXC transform pipeline. If it ever changes how it handles import declarations (e.g., converting them to require in some mode), the subsequent parseAst would stop finding ImportDeclaration nodes. The { sourcemap: false } option helps keep it fast but doesn't control the module output format. Worth keeping an eye on if OXC's defaults change.
There was a problem hiding this comment.
No code change here. I verified parseAst rejects TypeScript syntax directly, so the OXC transform remains necessary before parsing route modules. The current path keeps import declarations available for extraction; if OXC defaults ever changed that, the CSS import tests would need to catch it.
| // CSS file resolution is expected to be environment-neutral; the | ||
| // browser fetches the returned source href even though this resolve | ||
| // runs while generating the RSC entry. | ||
| const resolved = await this.resolve(specifier, importerPath, { skipSelf: true }); |
There was a problem hiding this comment.
This this.resolve runs in the RSC plugin context (since the load hook is on the RSC entry virtual module). The comment on line 1740-1742 correctly notes that CSS resolution is expected to be environment-neutral. This assumption holds today (.css files don't use export conditions), but it's a fragile assumption if Vite or the RSC plugin ever introduces environment-specific resolution for asset files.
The comment is good — just flagging that this is a load-bearing assumption that should be verified if the RSC plugin's environment handling changes.
There was a problem hiding this comment.
No code change here. The existing comment is intentionally documenting the load-bearing assumption: CSS source resolution is environment-neutral today, even though the resolve call runs while generating the RSC entry.
| } | ||
| } | ||
|
|
||
| if (hasUnsupportedTurbopackRules) { |
There was a problem hiding this comment.
This warning fires when any rule doesn't match the supported Tailwind-only shape, regardless of whether a Tailwind rule is also present. This is the correct behavior for mixed configs — the Codex review suggestion that this suppresses the warning when any Tailwind loader is detected is incorrect.
One readability nit: the message says "is not generally supported and will be ignored", which is accurate for the non-Tailwind rules but could be read as applying to all rules including the Tailwind one. Consider:
| if (hasUnsupportedTurbopackRules) { | |
| console.warn( | |
| '[vinext] next.config option "turbopack.rules" contains rules that are not supported and will be ignored. ' + | |
| "The known Tailwind CSS loader shape is translated automatically.", | |
| ); |
There was a problem hiding this comment.
Fixed by tightening the warning wording so it applies to unsupported rules specifically while preserving the Tailwind translation note.
| const candidate = path.join(projectRoot, name); | ||
| if (!fs.existsSync(candidate)) continue; | ||
|
|
||
| if (path.basename(candidate) === ".postcssrc") { |
There was a problem hiding this comment.
The .postcssrc handling reads the file here and parses it inline if it starts with {. The earlier review noted a double-read concern with readJsonFile. Looking at the current code, this path uses parseJsonConfigContent directly on the content string, so there's no double-read — the content is read once and parsed inline. Good.
| export function collectAppRouteModuleFiles( | ||
| route: AppRoute, | ||
| options: RouteModuleFilesOptions = {}, | ||
| ): string[] { |
There was a problem hiding this comment.
The module file enumeration in collectAppRouteModuleFiles differs from collectAppRouteAssetModuleFiles in two ways: (1) it includes routePath (route handlers) and (2) the ordering puts pagePath before layouts. The asset variant puts layouts before pages (for CSS cascade order) and omits route handlers.
This ordering difference is important and intentional, but it's non-obvious because both functions build very similar-looking arrays. Consider adding a brief comment in collectAppRouteModuleFiles noting that ordering doesn't matter here (it's used for import registration, not cascade ordering), to contrast with the asset variant's doc comment.
There was a problem hiding this comment.
Fixed by adding a comment above collectAppRouteModuleFiles that its ordering is for route-module registration, not CSS cascade ordering.
| const entries = await Promise.all( | ||
| routes.map(async (route) => { | ||
| const routeId = getRouteAssetId(route); | ||
| const cssHrefs = await collectDevCssHrefsForFiles( |
There was a problem hiding this comment.
Each route's CSS collection runs independently via Promise.all, which is good for parallelism. But routes that share a layout (e.g., / and /dashboard both use layout.tsx) will scan the same layout file independently. The DevCssImportsCache prevents re-reading/re-parsing the file, but the per-route work of walking the transitive graph from the layout still happens for every route.
This is fine for the current dev-only use case. In the future, if startup latency becomes a concern for large apps (100+ routes sharing deep layout trees), you could memoize at the route-module level ("CSS hrefs for this specific file and its transitive imports") rather than at the individual file scan level. The cache already partially achieves this since the scan results are cached, but the graph traversal itself is repeated.
There was a problem hiding this comment.
No code change here. Route-level memoization would add another cache layer for a speculative dev-startup optimization. The current cache already avoids repeated file reads/parses; I would leave graph-walk memoization until there is measured startup pressure from large shared layouts.
|
Review submitted successfully on PR #1130 with 10 inline comments. Here's a summary: Overall assessment: Well-structured PR with good separation of concerns. Most items from previous reviews were already addressed in the current code. Key findings:
Inline comments covered:
|
Review feedback pointed out duplicated record guards and a few unclear dev-only CSS tradeoffs. The duplicated guards made small config parsing helpers drift-prone, and the comments/messages left room for misreading unsupported Turbopack rules and dev stylesheet placement. Extract the record guard into a shared utility, tighten the PostCSS plugin resolver callback type, clarify the Turbopack warning, and document the intentional dev CSS cache and route-body stylesheet tradeoffs.
The Turbopack mixed-rules warning text was clarified after review, but the unit test still asserted the old wording. CI failed because the behavior was correct but the expected substring was stale. Update the assertion to match the clarified unsupported-rules warning while preserving the check that mixed Tailwind and unsupported Turbopack rules still emit a warning.
Some PR tests asserted incidental warning copy or exact rendered tag serialization. That made harmless wording or renderer formatting changes fail tests even though the behavior contract was unchanged. Assert emitted warning presence, structured compatibility statuses, extracted stylesheet hrefs, and throwing behavior instead of pinning human-facing prose.
|
/bigbonk review |
There was a problem hiding this comment.
Review
This is a well-architected PR. The separation of concerns is clean: css-config-compat.ts owns the Turbopack-to-PostCSS translation decision tree, postcss.ts owns plugin normalization and resolution, dev-css-imports.ts owns the bounded static import scanner, route-asset-manifest.ts joins routes with CSS, and app-route-module-files.ts extracts the shared module enumeration. The registerRouteModules deduplication via collectAppRouteModuleFiles is a nice cleanup.
Previous review concerns have been addressed well:
stripSourceCommentsreplaced entirely bytransformWithOxc+parseAst(correct approach)resolvePostcssPlugintype guard throws on undefined (line 264)- Conditional
stylesemission avoids empty arrays in production (line 121) Promise.allfor transitive source imports (line 310-311)- Doc comments added for module file ordering and cascade intent
- Dev CSS
<link>trade-off documented in route wiring (lines 786-790) - Cache validation tradeoff documented (lines 289-291)
I have two minor items below — neither is blocking. Approving.
|
|
||
| function isCssSpecifier(specifier: string): boolean { | ||
| const [pathname] = specifier.split("?", 1); | ||
| return CSS_EXTENSIONS.has(path.extname(pathname)); |
There was a problem hiding this comment.
Minor: path.extname is called on pathname after the query-string split, which is correct. But splitCssSpecifier (used at the call sites for resolveCssImportHref and resolveSourceImportPath) splits on ? — while isCssSpecifier does its own split("?", 1). The double-split is harmless but slightly redundant. Not worth changing, just noting for future readers.
| context: DevCssResolutionContext, | ||
| cache: DevCssImportsCache = new Map(), | ||
| ): Promise<string[]> { | ||
| async function collect(filePath: string, ancestors: ReadonlySet<string>): Promise<string[]> { |
There was a problem hiding this comment.
The cycle detection via ancestors (immutable per call chain) correctly prevents infinite recursion, and the Promise.all parallelizes the fan-out at each level. One subtlety: the top-level Promise.all on filePaths (line 316) combined with the per-level Promise.all here means that in the worst case, the entire transitive import graph for all root files fans out concurrently. For large layouts this is fine since the cache deduplicates and file counts are bounded. Just noting that the concurrency is unbounded — if dev startup ever shows pressure here, a concurrency limiter on the stat+read+parse path would be the place to look.
| * parallel) all await the same in-flight scan rather than each starting their own. | ||
| */ | ||
| export const postcssCache = new Map<string, Promise<{ plugins: unknown[] } | undefined>>(); | ||
| export const postcssCache = new Map<string, Promise<PostcssConfigInfo | undefined>>(); |
There was a problem hiding this comment.
The postcssCache is module-level state, which means it persists across Vite environments (RSC/SSR/Client config() hooks). This is intentional and correct — the cache key is projectRoot, and the PostCSS config is the same across environments. The comment on lines 64-68 explains this well.
| } | ||
| } | ||
|
|
||
| if ( |
There was a problem hiding this comment.
The three-way condition here (tailwindTurbopackCssLoader && !hasTailwindPostcssConfig && !hasUserTailwindVitePlugin) is the right gate. It correctly avoids double-injection when:
- The user already has
@tailwindcss/postcssin their PostCSS config - The user has
@tailwindcss/viteas a Vite plugin - The user has explicit
css.postcssin Vite config (warns instead of injecting) - The project has an opaque config like
.postcssrc.yml(warns instead of injecting)
Good decision tree.
| ...route.forbiddenPaths, | ||
| route.unauthorizedPath, | ||
| ...route.unauthorizedPaths, | ||
| route.pagePath, |
There was a problem hiding this comment.
In the asset variant, pagePath is listed last (after all layouts, templates, and boundary modules). This ensures layout CSS appears before page CSS in the collected hrefs. The main collectAppRouteModuleFiles puts pagePath first (line 14), which doesn't matter there since it's used for import registration, not cascade ordering.
The comment at line 7-8 and the doc comment at line 44-48 both explain this distinction. Good.
|
Review submitted successfully. Here's a summary: Approved with 5 inline comments (all informational, none blocking). Key findings:
Inline comments covered:
|
2 participants
Review dashboard
RouteAssetManifest.css-config-compat.ts,postcss.ts,route-asset-manifest.ts,dev-css-imports.ts.Why
The compatibility contract is that when a Next.js app expresses styling through configuration and App Router module conventions that Next.js accepts, Vinext should either honor that shape or leave it alone with a clear warning. It should not silently ignore supported Next patterns, and it should not fix one config shape by erasing another.
resolveCssConfigCompatibility()translates only safe Next/Tailwind shapes intocss.postcss.postcss.config.jsonand string plugin forms are normalized; opaque configs are preserved.RouteAssetManifestowns dev route CSS hrefs instead of adding CSS facts to route semantics.The current route asset implementation is intentionally a dev-only bridge. It joins route module entrypoints with a bounded static CSS discovery pass, but the boundary is designed so a future Vite/Rollup-backed manifest can own CSS, chunks, preloads, fonts, and hashed production assets.
Behavior change
turbopack.rules+@tailwindcss/webpack@tailwindcss/postcsswhen safe.postcss.config.json.postcssrc.ymlsrc/app/layout.tsximports"#/app/globals.css"basePathMaintainer review path
packages/vinext/src/plugins/css-config-compat.tspackages/vinext/src/plugins/postcss.tspostcss.config.json, string/object plugin normalization, disabled plugins, malformed JSON, and opaque config handling.packages/vinext/src/config/next-config.tsturbopack.rules.packages/vinext/src/server/app-route-module-files.tspackages/vinext/src/server/route-asset-manifest.tspackages/vinext/src/server/dev-css-imports.tspackages/vinext/src/index.tspackages/vinext/src/entries/app-rsc-manifest.tspackages/vinext/src/server/app-page-route-wiring.tsxstyles, andbasePathhref rendering.Tests
tests/tailwind-config.test.ts@tailwindcss/viteprecedence, explicit PostCSS preservation, opaque YAML preservation.tests/postcss-resolve.test.tspostcss.config.json, string/object plugin forms, malformed JSON, config priority, disabled plugins, Tailwind detection, opaque YAML.tests/dev-css-imports.test.ts/@fs.tests/route-asset-manifest.test.tstests/nextjs-compat/app-css.test.tssrc/app/layout.tsx.tests/app-page-route-wiring.test.tsbasePathstylesheet href prefixing without double-prefixing.tests/check.test.ts,tests/next-config.test.ts,tests/shims.test.ts,tests/entry-templates.test.ts,tests/app-router.test.tsCommands run:
vp check— passed with the existingrequest-pipeline.tswarning.vp test run tests/tailwind-config.test.ts tests/postcss-resolve.test.tsvp test run tests/route-asset-manifest.test.ts tests/dev-css-imports.test.tsvp test run tests/app-router.test.ts -t "generateRscEntry"vp test run tests/app-router.test.tsvp test run tests/app-page-route-wiring.test.ts tests/app-page-element-builder.test.ts tests/next-config.test.ts tests/tailwind-config.test.ts tests/app-router.test.tsvp test run tests/dev-css-imports.test.tsRisk / compatibility
turbopack.rulesremains generally unsupported except for the known Tailwind CSS loader shape. Explicit Vite CSS config is preserved.Non-goals
@tailwindcss/vitefrom Vinext's config hook.StaticSegmentGraphor route semantics.RouteAssetManifestfor chunks, preloads, fonts, and hashed CSS assets.Next.js references
postcss.config.json