Optimize class attributes built from CSS module classes - #3331
Open
DylanPiercey wants to merge 1 commit into
Open
Optimize class attributes built from CSS module classes#3331DylanPiercey wants to merge 1 commit into
DylanPiercey wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: efbb1a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3331 +/- ##
========================================
Coverage 94.29% 94.30%
========================================
Files 388 389 +1
Lines 53080 53401 +321
Branches 4231 4296 +65
========================================
+ Hits 50051 50358 +307
- Misses 3001 3013 +12
- Partials 28 30 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A `class` value assembled from CSS module classes (a stylesheet import or a `<style/name>` object) previously bailed to the runtime `classValue` walker, since `styles.foo` is a member expression the constant folder can't resolve. Recognize these as known class strings so the optimizer can treat them like static tokens: HTML concatenates them directly, DOM applies constant classes once at mount and toggles the conditional ones in place, and `_attr_class_item` handles a class that resolves to multiple tokens (eg from `composes:`). In development a class that resolves to a non-string logs a warning; the wrapper is compiled away in optimized builds.
DylanPiercey
force-pushed
the
claude/marko-idle-review-feedback-7ri0ar
branch
from
July 3, 2026 22:18
55ed6b4 to
efbb1a7
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughThis PR optimizes Changes
Sequence Diagram(s)sequenceDiagram
participant Template as Marko Template
participant Analyzer as native-tag analyze
participant CSSUtil as css-module-class util
participant Codegen as class attr codegen
participant Runtime as _attr_class_item / _class_module_value
Template->>Analyzer: class=[styles.card, {[styles.active]: on}]
Analyzer->>CSSUtil: isCSSModuleClass(expr, scope)
CSSUtil-->>Analyzer: known CSS module member
Analyzer->>Codegen: buildClassAttrExpression(meta, cssScope)
Codegen->>Codegen: buildCSSModuleClassExpression
Codegen-->>Runtime: apply constant string once
Codegen-->>Runtime: update toggles with _attr_class_item
Runtime->>Runtime: classList.toggle per token
Compact metadata Related issues: None specified Suggested labels: performance, runtime-tags, translator, needs-review Suggested reviewers: Marko core maintainers familiar with translator codegen and runtime DOM helpers Poem A rabbit hopped through stylesheet maze, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime-tags/src/dom/dom.ts (1)
90-106: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard
_attr_class_itemagainst missing CSS-module names
classModuleValuecan passundefinedhere in optimized builds, andname.indexOf(" ")now throws before any class is applied. Add a nullish guard before splitting/toggling so a typoed or renamed CSS-module export fails gracefully.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-tags/src/dom/dom.ts` around lines 90 - 106, The _attr_class_item helper currently assumes name is always a string, so classModuleValue can pass undefined and cause name.indexOf(" ") to throw before any class updates happen. Add a nullish guard at the start of _attr_class_item in dom.ts so missing CSS-module exports are ignored safely, and keep the existing classList.toggle logic for valid names.
🧹 Nitpick comments (1)
packages/runtime-tags/src/translator/visitors/tag/native-tag.ts (1)
1478-1496: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDev-mode CSS module warning is skipped for the simplest usage:
class={styles.foo}.
buildClassAttrExpressionbails immediately whenvalueisn't anObjectExpression/ArrayExpression(Line 1482-1484). This means a bare, unwrapped class value likeclass={styles.foo}(the most common CSS-module usage pattern) never reachesbuildCSSModuleClassExpression, so it's emitted as a rawcallRuntime(helper, branch)without theclassModuleValue/_class_module_valuedev-time validation wrapper that other shapes (arrays/objects) get. A typo'd class name here (e.g.styles.crad) will silently resolve toundefinedat runtime with zero dev diagnostics, then surface only as the runtime crash described in thedom/dom.tsreview of_attr_class_item.Consider also wrapping the lone-expression case with
classModuleValuewhenisCSSModuleClass(value, cssScope)is true, for consistent dev coverage.♻️ Sketch of extending coverage to the lone-expression case
function buildClassAttrExpression( value: t.Expression, cssScope: CSSModuleScope, ) { + if (isCSSModuleClass(value, cssScope)) { + return callRuntime("_attr_class", classModuleValue(value)); + } + if (value.type !== "ObjectExpression" && value.type !== "ArrayExpression") { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-tags/src/translator/visitors/tag/native-tag.ts` around lines 1478 - 1496, The lone CSS-module class case is skipped in buildClassAttrExpression, so class={styles.foo} never gets the dev-time classModuleValue/_class_module_value validation wrapper. Update buildClassAttrExpression in native-tag.ts to also handle a bare expression when isCSSModuleClass(value, cssScope) is true, and route it through buildCSSModuleClassExpression or the same wrapper logic used for object/array shapes so simple CSS-module usage gets consistent diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/runtime-tags/src/translator/core/style.ts`:
- Around line 95-102: The CSS-module registration in style translation is too
broad because registerStyleModuleVar(node.var.name) runs even when there is no
resolved import path. Update the analyze logic in style.ts so the var name is
only registered when importPath is truthy, and keep the check tied to the
existing node.var handling in analyze and the isCSSModuleClass lookup path. This
ensures only style tags that actually emit a backing import are treated as
CSS-module objects.
In `@packages/runtime-tags/src/translator/util/css-module-class.ts`:
- Around line 52-62: The `<style/var>` check in `css-module-class.ts` is
matching by identifier name alone, which can incorrectly fold shadowed or
unrelated bindings into CSS module output. Update the
`getProgram().node.extra?.styleModuleVars` flow to carry and compare the
analyzed binding identity in `cssModuleClass` rather than `expr.object.name`,
and keep the existing `scope.getBinding(...)` module-binding validation as the
fallback for translate-time matching.
---
Outside diff comments:
In `@packages/runtime-tags/src/dom/dom.ts`:
- Around line 90-106: The _attr_class_item helper currently assumes name is
always a string, so classModuleValue can pass undefined and cause name.indexOf("
") to throw before any class updates happen. Add a nullish guard at the start of
_attr_class_item in dom.ts so missing CSS-module exports are ignored safely, and
keep the existing classList.toggle logic for valid names.
---
Nitpick comments:
In `@packages/runtime-tags/src/translator/visitors/tag/native-tag.ts`:
- Around line 1478-1496: The lone CSS-module class case is skipped in
buildClassAttrExpression, so class={styles.foo} never gets the dev-time
classModuleValue/_class_module_value validation wrapper. Update
buildClassAttrExpression in native-tag.ts to also handle a bare expression when
isCSSModuleClass(value, cssScope) is true, and route it through
buildCSSModuleClassExpression or the same wrapper logic used for object/array
shapes so simple CSS-module usage gets consistent diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2493c2ed-af98-4196-9c9a-fa8e7bc89be3
⛔ Files ignored due to path filters (6)
packages/runtime-tags/src/__tests__/fixtures/attr-class-css-module-style/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/attr-class-css-module-style/__snapshots__/html.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/attr-class-css-module-style/__snapshots__/html.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/attr-class-css-module/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/attr-class-css-module/__snapshots__/html.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/attr-class-css-module/__snapshots__/html.bundle.jsis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (18)
.changeset/css-module-class-optimization.md.sizes.json.sizes/dom.jspackages/runtime-tags/src/__tests__/fixtures/attr-class-css-module-style/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/attr-class-css-module-style/template.markopackages/runtime-tags/src/__tests__/fixtures/attr-class-css-module-style/test.tspackages/runtime-tags/src/__tests__/fixtures/attr-class-css-module/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/attr-class-css-module/styles.module.csspackages/runtime-tags/src/__tests__/fixtures/attr-class-css-module/template.markopackages/runtime-tags/src/__tests__/fixtures/attr-class-css-module/test.tspackages/runtime-tags/src/__tests__/fixtures/define-tag-for-attribute-tag/sizes.jsonpackages/runtime-tags/src/common/helpers.tspackages/runtime-tags/src/dom.tspackages/runtime-tags/src/dom/dom.tspackages/runtime-tags/src/html.tspackages/runtime-tags/src/translator/core/style.tspackages/runtime-tags/src/translator/util/css-module-class.tspackages/runtime-tags/src/translator/visitors/tag/native-tag.ts
Comment on lines
+95
to
+102
| // Record the `var` name (eg `<style/styles>`) so the `class` attribute | ||
| // optimizer recognizes `styles.foo` as a CSS module class. Done in analyze | ||
| // because translate removes this tag and emits the backing import late, | ||
| // after which the name is no longer resolvable as a binding. | ||
| if (t.isIdentifier(node.var)) { | ||
| registerStyleModuleVar(node.var.name); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect getStyleImportPath to see when it can be falsy while node.var is set.
rg -n "function getStyleImportPath" -A 40 packages/runtime-tags/src/translatorRepository: marko-js/marko
Length of output: 3536
🏁 Script executed:
#!/bin/bash
sed -n '1,180p' packages/runtime-tags/src/translator/core/style.ts
printf '\n---\n'
sed -n '180,340p' packages/runtime-tags/src/translator/core/style.ts
printf '\n---\n'
sed -n '340,460p' packages/runtime-tags/src/translator/core/style.tsRepository: marko-js/marko
Length of output: 11881
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- css-module-class ---'
sed -n '1,220p' packages/runtime-tags/src/translator/util/css-module-class.ts
printf '\n%s\n' '--- usages ---'
rg -n "registerStyleModuleVar|isCSSModuleClass" packages/runtime-tags/src/translator -A 3 -B 3
printf '\n%s\n' '--- style import path callers ---'
rg -n "styleImportPath" packages/runtime-tags/src/translator -A 3 -B 3Repository: marko-js/marko
Length of output: 11712
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,220p' packages/runtime-tags/src/translator/util/css-module-class.ts
printf '\n---\n'
rg -n "registerStyleModuleVar|isCSSModuleClass" packages/runtime-tags/src/translator -A 4 -B 4Repository: marko-js/marko
Length of output: 11545
Guard CSS-module registration on a resolved import path
registerStyleModuleVar(node.var.name) should only run when importPath is truthy. isCSSModuleClass() treats any registered <style var> as a CSS-module object, so a style tag with var but no emitted import can make unrelated styles.foo accesses look static.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/runtime-tags/src/translator/core/style.ts` around lines 95 - 102,
The CSS-module registration in style translation is too broad because
registerStyleModuleVar(node.var.name) runs even when there is no resolved import
path. Update the analyze logic in style.ts so the var name is only registered
when importPath is truthy, and keep the check tied to the existing node.var
handling in analyze and the isCSSModuleClass lookup path. This ensures only
style tags that actually emit a backing import are treated as CSS-module
objects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A
classvalue assembled from CSS module classes (a stylesheet import or a<style/name>object) previously bailed out of the compile-time optimizer, sincestyles.foois a member expression the constant folder can't resolve, so the whole array/object was handed to the runtimeclassValuewalker on every render.This recognizes a member access on a CSS module as a known class string (the value is filled in by the bundler; we only need to know it's a string) and treats these like static tokens:
_attr_class_itemnow handles a class that resolves to multiple space-separated tokens (eg fromcomposes:), whichclassList.togglewould otherwise reject.Detection covers both a default/namespace import from a stylesheet source and a
<style/var>object, and is phase-consistent (the<style>var is registered in analyze, before translate removes the tag).In development each CSS module class access is wrapped so a non-string (a typo or a class missing from the stylesheet) logs a warning; the wrapper is compiled away in optimized builds.
The DOM runtime grows ~105 bytes min / 41 brotli, from the
_attr_class_itemmulti-token handling and the debug-only assertion helper.Checklist:
Generated by Claude Code