You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Code Review — refactor(styled-jsx): Hash dynamic styles from source text
Nice, focused change. Replacing drop_span(expr.clone()).hash(...) with source-text hashing is a solid improvement, and the diff is easy to follow. Summary of my review below.
👍 What's good
Removes AST hygiene coupling. The old drop_span(expr.clone()).hash(&mut hasher) still hashed each Ident's SyntaxContext, so the resulting dynamic-style hash depended on resolver/hygiene state and diverged from Babel. Hashing the raw source text is deterministic and matches Babel's behavior, which is the right call.
Small perf win. It drops a full expr.clone() (plus the drop_span fold) per dynamic style in favor of a single source-map snippet lookup. span_to_snippet is called once per style, so cost is negligible.
Tagged templates handled. The clone at visitor.rs:679 preserves the original span, so span_to_snippet still recovers the backtick-delimited source. Good that this path is covered.
🔎 Behavioral change worth calling out
The switch to source-text hashing means two dynamic styles with identical source text but different variable bindings now collide to the same hash. This is visible in the attribute-generation-modes fixture: Test4 and Test6 previously had distinct ids (8db39ad50e58394a / 4375826949b9259f) and now both resolve to 98567cd857d5ceb8.
This looks intentional (identical CSS templates dedupe to one style id, matching Babel), and the generated CSS is identical so runtime behavior is preserved — but since these hashes appear in emitted output, it's a user-visible change. Worth a CHANGELOG entry noting dynamic-style hash values change, in case any downstream consumers pin them.
💬 Minor / optional
Two divergent hash-input formats. The primary path hashes the inner source (`...` with backticks stripped), while the fallback reconstructs quasi + "${" + exprSnippet + "}". These produce different byte streams for the same template, so the fallback yields a different (though still deterministic) hash than the primary path. Within a single compilation the path is consistent, so this is fine in practice — but a one-line comment explaining that the reconstruction is a best-effort fallback (used only when the outer span can't be resolved) would help future readers.
Silent empty writes in the fallback. In the fallback loop, if an inner expr's span_to_snippet fails it writes nothing between ${ and }. Two different expressions that both fail to resolve would then hash identically. Very low risk (synthetic spans only), but noting it since it's a silent collision path.
DefaultHasher stability (pre-existing). SipHash output isn't guaranteed stable across Rust toolchain versions. Since the fixtures now pin these hash values, a future toolchain bump could churn them. Not introduced by this PR, just flagging as a latent maintenance cost.
✅ Tests
Fixture outputs are updated consistently across both swc and lightningcss backends, and CI Test - ubuntu-latest is green. Coverage looks adequate for the change.
Overall LGTM — the only thing I'd add before merge is a changelog note about the changed hash values.
Review: refactor(styled-jsx): Hash dynamic styles from source text
Nice, focused refactor. Dropping drop_span(expr.clone()).hash(...) in favor of hashing the original source text removes the reliance on AST-shape hashing and, as the PR notes, tracks Babel's behavior more closely. The fixture churn is purely regenerated hash values, which is exactly what you'd expect. Overall this looks correct and I'd be happy to see it land. A few observations below.
Correctness / behavior
Behavior change worth calling out explicitly. Hashing source text means the hash is now sensitive to formatting inside interpolations that the old AST-based hash normalized away — e.g. ${color} vs ${ color }, or a comment inside the template, now produce different dynamic style ids, whereas drop_span(...).hash() treated them identically. This is intentional (it's what makes it match Babel) and safe as long as the same transform runs for both the SSR and client passes, but it's a hash-value breaking change vs. previous releases. Might be worth a line in the changeset/PR body so downstream consumers aren't surprised.
Escaped backticks are handled correctly. I checked tpl-escape-2 (which contains content: "\";). strip_prefix('`')/strip_suffix('`')` only peel the outer delimiter pair, so the escaped inner backtick is preserved in the hashed content. 👍
Fallback path can lose expression content under synthetic spans. In the reconstruction branch, each interpolation is emitted as ${ + span_to_snippet(expr.span()) + }. If the surrounding span_to_snippet on the whole Tpl failed (dummy/synthetic spans), the inner expr snippets will typically fail too, so distinct templates like `a:${x}` and `a:${y}` would both hash as a:${} — a latent collision. For styled-jsx operating on parsed user source this is effectively unreachable, so I wouldn't block on it, but a short comment documenting the "spans are always real here" assumption (or hashing a discriminator when the snippet is empty) would help the next reader.
Primary vs. fallback consistency. The two paths hash slightly different byte streams (raw source between backticks vs. quasi.raw + reconstructed ${…}). They agree when spans resolve, and within a single compilation only one path is taken per template, so there's no intra-run inconsistency. Just noting it since the two encodings aren't identical.
Style (minor)
for i in 0..tpl.quasis.len() with tpl.quasis[i] / tpl.exprs.get(i) works, but tpl.quasis.iter().enumerate() would be more idiomatic and avoid the manual index / bounds indexing. Not important.
Tests
Coverage comes via the fixture snapshots (the repo's convention), and the regenerated outputs exercise the interesting cases (dynamic-only, static+dynamic, escaped backticks, placeholders). A tiny direct unit test for hash_expr_for_style covering the escaped-backtick and no-source-map fallback paths would nail down the edge cases the fixtures can't easily reach, but it's optional.
Nits / sanity checks
Please confirm cargo clippy -D warnings is green — the newly added SourceMapper / Spanned imports need to be actually exercised (they are: span_to_snippet and .span()), so this should be fine, just worth a glance once CI settles.
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
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.
Summary