-
-
Notifications
You must be signed in to change notification settings - Fork 502
fix(core, react-form): Avoid set state error in React by avoiding set… #1250
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
fix(core, react-form): Avoid set state error in React by avoiding set… #1250
Conversation
View your CI Pipeline Execution ↗ for commit 6b91219.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1250 +/- ##
=======================================
Coverage 88.86% 88.86%
=======================================
Files 28 28
Lines 1275 1275
Branches 332 332
=======================================
Hits 1133 1133
Misses 127 127
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 think I'd rather add the defaultValue
setting inside of mount
.
Also, ideally we don't have to set this flag to do that. If we set it inside of mount
, do all tests still pass on all adapters?
cc55fba
to
e04526a
Compare
All tests passing on adapters, moved to simpler version of setting field value on mount function. Kept test within React that specifically tests the issue. |
e04526a
to
5c4450e
Compare
Updated! |
… react set state on render error + include test in react package
f4f1055
to
a2d03f9
Compare
LGTM 🚀 Just had to fix the conflict🤟 |
…ting default value until mount #1134
Due to the FieldApi constructor setting defaultValue in the constructor we violate: https://react.dev/link/setstate-in-render
UPDATE: We're going with option 2 below: moving set default value to mount function.
To avoid this I added a new flag which will prevent setting the default value within the constructor:
Within the react react-form package, we then set that flag true and do the following
Please let me know if you prefer this implementation ^^
Or a different implementation where we can simply move the following code into the mount function of the FieldApi class:
form/packages/form-core/src/FieldApi.ts
Lines 999 to 1003 in 135b886
This also solves the problem, though now we'd be affecting all other packages not just the React one. Tests pass for either implementation!
Thanks in advance!