-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Form lib] Memoize form hook object and fix hook array deps #71237
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
@sebelga awesome work!
Definitely tricky to spot runtime staleness errors if there are any 😅 but the proposed changes all look good to me and I'm glad we could tackle this issue!
I clicked through different forms in the mappings editor and in ingest processors editor and they seemed to work as expected.
Thanks for the review @jloleysens ! Yes it is tricky to detect. I had quite a few console.log() to detect infinite re-renders, and got a few of them solved. I admit that there is a problem with the data flow inside the lib that I'd like to address in |
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.
LGTM! My review was testing only. Tested ingest node pipelines, index templates and component template forms and did not spot any regressions.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@sebelga I believe the test failures are valid, as the |
@elasticmachine merge upstream |
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.
Thanks so much for the updates! This is much cleaner. I did notice a few react warnings pop up in the step_about_rule
that I don't think were there before (just quickly checked against a master). Not sure if they're related to the changes?
Thanks for the review @yctercero ! I will check those React warnings 👍 |
isNew: false, | ||
}; | ||
setMyStepData(myDefaultValues); | ||
setFieldValue(form, schema, myDefaultValues); |
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.
@sebelga we have this pattern in several of these "rule creation step" forms. Do you intend to change the others after we verify this one?
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.
@rylnd We will have to. setFieldValue
was probably a hack that you had to create because of an unexpected behaviour on the form lib. Now that all the handlers are memoized and the form
object too, this hack is not necessary anymore. I will review it tomorrow with Patryk.
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.
@sebelga yes, absolutely! Very excited to clean things up once this is merged 🙏
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.
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.
…memoize # Conflicts: # x-pack/plugins/security_solution/public/cases/components/add_comment/index.tsx # x-pack/plugins/security_solution/public/cases/components/create/index.tsx # x-pack/plugins/security_solution/public/cases/components/user_action_tree/user_action_markdown.tsx
💚 Build SucceededBuild metricsasync chunks size
miscellaneous assets size
page load bundle size
History
To update your PR or re-run it, just comment with: |
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.
Case files and manual testing of the case forms all LGTM, thanks!
Thanks for the review @stephmilovic ! |
* master: [Form lib] Memoize form hook object and fix hook array deps (elastic#71237) [uiActions] Support emitting nested triggers and actions (elastic#70602) add short sleep before clicking Remove on sample data (elastic#71104) Fixed the beta badge layout. (elastic#71835) Restores task for downloading Chromium builds (elastic#71749) [logging] Format new platform json logging to ECS (elastic#71138) add policy details and update SO limit requests (elastic#71789) Convert vis_type_vega to Typescript (elastic#68915) [ML] Fix UI Actions context menu positioning for the Anomaly Swim Lane (elastic#71839)
…1237) (#71875) Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Patryk Kopycinski <[email protected]>
This PR correctly memoize the handlers and variables of the form lib hooks. It also declares the correct hooks deps to get us one step closer to close #49554
I have also updated the forms of index management to correctly declare their
useEffect
deps.How to test
There isn't an easy way to test. We need to go through different forms and make sure everything works as expected. I have already spent a large amount of time testing the index templates forms, mainly the mappings editor forms.