Skip to content

refactor(styled-jsx): Hash dynamic styles from source text#644

Merged
kdy1 merged 3 commits into
mainfrom
refactor/remove-styled-jsx-ast-hash
Jul 8, 2026
Merged

refactor(styled-jsx): Hash dynamic styles from source text#644
kdy1 merged 3 commits into
mainfrom
refactor/remove-styled-jsx-ast-hash

Conversation

@magic-akari

Copy link
Copy Markdown
Member

Summary

  • Remove the styled-jsx dependency on hashing SWC AST nodes.
  • Hash dynamic style template literals from their original source text, matching the Babel implementation more closely.
  • Update styled-jsx fixture outputs for the new dynamic style hash values.

@changeset-bot

changeset-bot Bot commented Jul 7, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 6c84cc9

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

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

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

  1. 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.
  2. 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.
  3. 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.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

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.

Thanks for the cleanup!

@magic-akari
magic-akari marked this pull request as ready for review July 8, 2026 02:54

@kdy1 kdy1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like a good idea. Thank you!

@kdy1
kdy1 merged commit 9d7587a into main Jul 8, 2026
11 checks passed
@kdy1
kdy1 deleted the refactor/remove-styled-jsx-ast-hash branch July 8, 2026 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants