-
Notifications
You must be signed in to change notification settings - Fork 36
NXT-8994: Adapted Enact code to respect React Compiler
#3379
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3379 +/- ##
===========================================
+ Coverage 83.24% 86.07% +2.82%
===========================================
Files 154 136 -18
Lines 7258 5551 -1707
Branches 1919 1439 -480
===========================================
- Hits 6042 4778 -1264
+ Misses 956 610 -346
+ Partials 260 163 -97 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
packages/ui/Skinnable/useSkins.js
Outdated
| if (parentVariants !== currentParentVariants) { | ||
| setCurrentParentVariants(parentVariants); | ||
| } |
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.
this causes an additional rerender on every parentVariants change. When parentVariants changes, the first render uses the old currentParentVariants, then setState triggers a second render with the new value
How about just dropping useMemo() ? Let me know your opinion.
"const effectiveVariants = determineVariants(defaultVariants, variants, skinVariants, parentVariants);"
packages/ui/Toggleable/Toggleable.js
Outdated
| instance.selected = propSelected; | ||
|
|
||
| if (propSelected && instance.selected !== propSelected) { | ||
| setInstance({selected: propSelected}); |
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.
i see an issue of potential inifnite loops because of setState on render.
How about using the derivedStateFromProps pattern?
| setInstance({selected: propSelected}); | |
| const [derivedState, setDerivedState] = useState(() => ({ | |
| prevPropSelected: null, | |
| selected: hook.selected | |
| })); | |
| if (propSelected !== derivedState.prevPropSelected) { | |
| const newSelected = (derivedState.prevPropSelected && propSelected == null) ? false : hook.selected; | |
| setDerivedState({ | |
| prevPropSelected: propSelected, | |
| selected: newSelected | |
| }); | |
| } | |
| const selected = derivedState.selected; |
Checklist
Issue Resolved / Feature Added
Adapted
Enactcode to respectReact CompilerResolution
eslint-plugin-react-hooks 7.xhasReact Compilerembedded. The code needs to be adapted to respectEslintrulesrefswere changed during render, and also cannot read.currentduring render. This is not allowed outsidehandlersoreffectsREFERENCEuseEffectis not recommendedAdditional Considerations
Some of the errors were silenced as they were considered false positives
Links
NXT-8994
Comments
Enact-DCO-1.0-Signed-off-by: Ion Andrusciac ion.andrusciac@lgepartner.com