-
-
Notifications
You must be signed in to change notification settings - Fork 80
feat: don't spread key in withUnistyles #862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes update the Changes
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/withUnistyles/withUnistyles.native.tsx (1)
94-100
: Runtime-changing keys will remount components
key={uniPropsKey || mappingsKey}
means the wrapped component is fully remounted whenever either mapping returns a different key between renders.
If those keys are derived from theme/runtime-dependent values (e.g. random IDs, timestamps, colour hashes) you may inadvertently reset component state on every theme change.Confirm the keys are:
- Stable for a given render cycle, and
- Change only when you want a remount.
If stability cannot be guaranteed, consider omitting
key
entirely outside list contexts, or gating the assignment:- key={uniPropsKey || mappingsKey} + key={uniPropsKey ?? mappingsKey ?? undefined}src/core/withUnistyles/withUnistyles.tsx (1)
65-69
: Potential unnecessary remount (same concern as native file)See remark in
withUnistyles.native.tsx
: changinguniPropsKey
/mappingsKey
between renders will force unmount / mount ofNativeComponent
, wiping its internal state. Validate key stability or drop it when not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/withUnistyles/types.ts
(1 hunks)src/core/withUnistyles/withUnistyles.native.tsx
(2 hunks)src/core/withUnistyles/withUnistyles.tsx
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (3)
src/core/withUnistyles/withUnistyles.native.tsx (1)
78-80
: Good extraction of reservedkey
propDestructuring
key
out of the HOC-level props prevents React’s “key is not a prop” warning. Nice fix.src/core/withUnistyles/withUnistyles.tsx (2)
30-32
: Mirrors native fix — looks goodSame reserved-prop extraction logic as in the native implementation.
35-38
: Filtering underscore-prefixed style keysThe extra
!key.startsWith("_")
guard hides private style fields from leaking to the DOM – nice defensive move.
Summary
Fixes error in the console while mapping key:
Summary by CodeRabbit