feat: optimize cache key generation for better performance

[divineniiquaye]

I tested this new cache system using bitwise against uniwind 1.3.0 and saw a 1.17× difference in performance, @Brentlok kindly review and check if it's worth merging

Summary by CodeRabbit

• Refactor
  • Improved internal style caching and key generation to reduce collisions and improve performance.
• Bug Fix
  • Styles that include data attributes are no longer reused from cache, ensuring correct recomputation and more reliable styling results.

[coderabbitai]

📝 Walkthrough

Walkthrough

Added a boolean hasDataAttributes to StylesResult and propagated it via emptyState and resolveStyles; cache key generation now uses className:stateFlags (bit flags). Cache lookup uses cache.get(cacheKey) ?? resolveStyles(...), and results with data attributes are not cached.

Changes

Cohort / File(s)	Summary
Native store updates packages/uniwind/src/core/native/store.ts	Added hasDataAttributes: boolean to StylesResult and emptyState. Replaced string-based cache key with className:stateFlags using bit flags for isDisabled/isFocused/isPressed. Cache retrieval changed to cache.get(cacheKey) ?? this.resolveStyles(...). Results where hasDataAttributes === true are not cached; resolveStyles now returns hasDataAttributes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• feat: support data attributes #339: Overlapping changes introducing hasDataAttributes and adjusting getStyles/resolveStyles signatures and cache key/behavior.

Poem

🐰 I count the bits, I hop and play,
className:flags guide my way,
when data-attributes flutter by,
I skip the cache and give a sigh,
nibble keys, then bound away.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: optimizing cache key generation using bit-flag operations for performance improvement, which aligns with the core modifications to the caching strategy in the store.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

No actionable comments were generated in the recent review. 🎉

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/uniwind/src/core/native/store.ts`:
- Around line 31-35: The cache key collapses undefined and false because
stateFlags only encodes truthy bits; update the cache key generation so
tri-state is preserved (e.g., add presence bits for each prop or include a
normalized representation of state) — change the logic around
stateFlags/cacheKey in store.ts (symbols: stateFlags, cacheKey, this.cache.get)
to include separate bits or a distinct marker for "defined vs undefined" for
isDisabled/isFocused/isPressed (or use a deterministic serialization of state)
so a cached result computed when state is omitted is not reused for an explicit
false (and vice versa).

[jpudysz]

Hey @divineniiquaye I've tested it couple of times and I can see small improvement in our benchmark.

Before: 81-83 ms

After: 80-83 ms

Not sure about the readability though. It looks a little bit worse and harder to maintain

[divineniiquaye]

@jpudysz I've updated the code to be readable, added missing hasDataAttributes field to StylesResult and initialize it in emptyState.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/uniwind/src/core/native/store.ts (1)

38-48: ⚠️ Potential issue | 🔴 Critical

Listener subscriptions accumulate on every cache hit — performance and memory regression.

With the ?? pattern, when the cache already contains the result, execution still falls through to lines 41-48. Every call to getStyles for the same cacheKey adds another UniwindListener.subscribe(…, { once: true }) closure. Over many renders this accumulates unbounded listeners, each holding a reference to cacheKey and result.dependencies. This is counterproductive for a performance-focused PR.

Guard the subscription so it only runs on a cache miss:

Proposed fix

-        const result = this.cache.get(cacheKey) ?? this.resolveStyles(className, componentProps, state)
+        const cached = this.cache.get(cacheKey)
+
+        if (cached !== undefined) {
+            return cached
+        }
+
+        const result = this.resolveStyles(className, componentProps, state)

         // Don't cache styles that depend on data attributes
         if (!result.hasDataAttributes) {

[Brentlok]

Thanks for the PR.

As @jpudysz mentioned, the trade-off here doesn't align with our project goals. The performance gain is too small to justify the negative impact on readability and maintainability. Since the current code isn't causing any issues, we prefer to keep it as is.

As noted in our README, please reach out to us before starting work on refactors so we can align on the approach first. I'm closing this for now.

[divineniiquaye]

Thanks for the PR.

As @jpudysz mentioned, the trade-off here doesn't align with our project goals. The performance gain is too small to justify the negative impact on readability and maintainability. Since the current code isn't causing any issues, we prefer to keep it as is.

As noted in our README, please reach out to us before starting work on refactors so we can align on the approach first. I'm closing this for now.

Interesting 🤔, but okay. I found more optimizations improvements was giving a shot to see the outcome.

This was quite expected, anyways thanks for reviewing it.