fix(build): honour experimental.lightningCssFeatures include/exclude - #1664
Conversation
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-structured change. The bitmask values all match lightningcss Features exactly (verified against lightningcss@1.31.1/node/targets.d.ts), the composite groups are correct, and the Vite css.transformer / css.lightningcss.include / css.lightningcss.exclude plumbing targets valid config keys.
A few observations worth noting, though none block merge:
Config interaction when both postcssOverride and useLightningcss are true: The current merge order at index.ts:1724-1755 spreads the lightningcss block first, then the postcss block. When both are truthy, the resulting config contains transformer: 'lightningcss' AND postcss: <override>. Vite ignores the postcss key when transformer is 'lightningcss', so this is functionally harmless — but it's a minor parity question worth tracking since Next.js presumably also ignores postcss when lightningcss is enabled.
Empty lightningcss: {} object when no features are configured: When useLightningcss: true but no lightningCssFeatures are set (both masks are 0), the code emits css: { transformer: 'lightningcss', lightningcss: {} }. The empty object is a no-op for Vite (it falls through to lightningcss defaults), so this is fine — just noting it's intentional.
Test coverage is excellent. The regression test exercises both include and exclude via real builds, and the unit tests cover individual features, composite groups, OR-merging, and unknown-name warnings. The makeResolved helper in next-config.test.ts was correctly updated with the new fields.
Overall: clean implementation, correct bitmask mapping, good parity with Next.js, good test coverage.
|
Review submitted. Here's a summary of the review: Approved. The implementation is correct:
I noted two minor non-blocking observations about the config interaction when both PostCSS and lightningcss are active (Vite ignores PostCSS in that case, so it's harmless) and the empty |
89f3814 to
90b37b8
Compare
|
Rebased on |
…1498) vinext silently ignored `experimental.lightningCssFeatures` because the options were never plumbed into Vite's CSS pipeline. `light-dark()` and other modern CSS features were still lowered to polyfills even when the user explicitly listed them under `exclude`. Resolve `experimental.useLightningcss` and convert the dash-case feature names from `experimental.lightningCssFeatures` into the numeric bitmask expected by lightningcss (mirroring Next.js's `featureNamesToMask` in `packages/next/src/build/webpack/loaders/lightningcss-loader/src/loader.ts` and `crates/next-core/src/next_config.rs`). When `useLightningcss` is on, forward `transformer: 'lightningcss'` plus the resolved `include`/`exclude` masks to Vite's `css.lightningcss` bridge. Adds a regression test that builds a fixture with `exclude: ['light-dark']` and asserts the function survives untouched in the emitted CSS, plus an inverse `include: ['light-dark']` case and unit coverage for the feature-name → bitmask helper. Ported from Next.js: test/e2e/app-dir/experimental-lightningcss-features/experimental-lightningcss-features.test.ts
90b37b8 to
ea6f1ec
Compare
|
Rebased on origin/main; resolved adjacent-field conflicts in |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — fix(build): honour experimental.lightningCssFeatures include/exclude
Reviewed the implementation against the lightningcss Features enum, the Next.js Rust reference, and Vite's CSS transformer dispatch. This is a clean, correct, well-tested change. No blocking issues.
Verified correct
Bitmask values (next-config.ts:1071-1098) — All 21 individual feature bits and 3 composite groups match lightningcss@1.31.1/node/targets.d.ts Features exactly (light-dark = 1048576, selectors = 31, media-queries = 448, colors = 1113088, etc.). The repo pins lightningcss 1.31.1, so the literals are accurate.
Feature-name mapping — Matches Next.js lightningcss_feature_names_to_mask in crates/next-core/src/next_config.rs name-for-name (all 24 dash-case keys identical). Good parity.
Vite plumbing (index.ts:1765-1796) — css.transformer: "lightningcss" + css.lightningcss.include/exclude are valid, correctly-typed keys: include/exclude come from BundleAsyncOptions and are not in Vite's LightningCSSOptions omit list. Confirmed in Vite source (chunks/node.js:26654) that transformer === "lightningcss" routes to compileLightningCSS, which never reads config.css.postcss — so emitting postcss alongside the lightningcss block when both are configured is a genuine no-op, not a silent footgun.
Warning parity — Warning when lightningCssFeatures is set without useLightningcss matches Next.js behavior.
Tests — tests/lightning-css-features.test.ts (2 tests, both directions) and the 9 unit tests in next-config.test.ts pass locally. vp check clean on both source files. The makeResolved helper was correctly updated with the two new fields.
Non-blocking observations
-
Unknown-name handling is a documented divergence. Next.js's Rust path
bail!s (hard error) on an unknown feature name; vinext warns and skips. This is called out in the code comment as deliberate (don't break the build on a stray name). Reasonable, but worth noting Next.js users who rely on the error to catch typos won't get the same signal here. Not a blocker. -
useLightningcssswitches the entire CSS pipeline from PostCSS to lightningcss, matching Next.js — but if a user has a custom PostCSS config and flipsuseLightningcss: true, their PostCSS plugins are silently bypassed by Vite. This is Next.js-equivalent behavior and not introduced by this PR, just inherent to the feature.
Suggestion (optional, future)
The regression test uses raw build() with a single rollupOptions.input rather than createBuilder().buildApp(). That's fine for exercising the client CSS transform, but it doesn't cover the full multi-environment build path. Not worth blocking on — the client environment is where CSS emission happens.
LGTM.
|
Review posted to PR #1664. SummaryI reviewed PR #1664 ( What I verified
Non-blocking notes I called out
LGTM. |
1 participant
Summary
Fixes #1498. vinext silently ignored
experimental.lightningCssFeaturesbecause the option was never plumbed into Vite's CSS pipeline.light-dark()and similar modern features were always lowered to thevar(--lightningcss-light, ...)polyfill, even when the user listed them underexclude.experimental.useLightningcssandexperimental.lightningCssFeaturesinresolveNextConfig, converting the dash-case feature names into the lightningcssFeaturesbitmask (mirroring Next.js'sfeatureNamesToMaskin the webpack loader and the Rustlightningcss_feature_names_to_maskhelper).useLightningcssis enabled, forwardtransformer: 'lightningcss'plus the resolvedinclude/excludemasks to Vite'scss.lightningcssconfig so they actually reach the lightningcsstransform()call.lightningCssFeaturesis set withoutuseLightningcss.Test plan
tests/lightning-css-features.test.tsbuilds a fixture withexclude: ['light-dark']and assertslight-dark(survives in the emitted CSS, plus an inverseinclude: ['light-dark']case that asserts the polyfill is emitted. Verified theexcludecase fails onmainand passes with this change.lightningCssFeatureNamesToMaskandresolveNextConfig(individual features, composite groups, OR-merging, unknown-name warning, anduseLightningcsswarning parity).pnpm run check(format + lint + type) — clean.tests/css-media-query-target.test.ts,tests/css-modules-data-urls.test.ts,tests/node-modules-css.test.ts,tests/postcss-resolve.test.ts,tests/scss.test.ts).Ported from Next.js:
test/e2e/app-dir/experimental-lightningcss-features.