[Form lib] Memoize form hook object and fix hook array deps by sebelga · Pull Request #71237 · elastic/kibana

sebelga · 2020-07-09T13:06:47Z

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.

elasticmachine · 2020-07-09T13:08:15Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

jloleysens

@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.

sebelga · 2020-07-09T14:37:51Z

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 7.10 to make changes more predictable.
But this PR is already a good step forward to make things cleaner for the consumer.

alisonelizabeth

LGTM! My review was testing only. Tested ingest node pipelines, index templates and component template forms and did not spot any regressions.

sebelga · 2020-07-09T15:47:30Z

@elasticmachine merge upstream

sebelga · 2020-07-09T16:56:19Z

@elasticmachine merge upstream

alisonelizabeth · 2020-07-09T19:33:02Z

@elasticmachine merge upstream

alisonelizabeth · 2020-07-09T21:54:03Z

@elasticmachine merge upstream

alisonelizabeth · 2020-07-09T23:14:51Z

@elasticmachine merge upstream

alisonelizabeth · 2020-07-10T00:43:18Z

@sebelga I believe the test failures are valid, as the security_solution plugin uses the form lib and the tests are not failing on other tracked branches.

sebelga · 2020-07-13T12:56:49Z

@elasticmachine merge upstream

yctercero

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?

sebelga · 2020-07-13T16:36:12Z

Thanks for the review @yctercero ! I will check those React warnings 👍

rylnd · 2020-07-13T17:50:34Z

x-pack/plugins/security_solution/public/detections/components/rules/step_about_rule/index.tsx

-        isNew: false,
-      };
-      setMyStepData(myDefaultValues);
-      setFieldValue(form, schema, myDefaultValues);


@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?

@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.

@sebelga yes, absolutely! Very excited to clean things up once this is merged 🙏

++ @rylnd's sentiment! 🙂 Between this and #60602 we'll really be able to clean things up on our end. Thanks for all the work here @sebelga!

Thanks @spong ! And good that you bring #60602 back to my attention 👍

…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

kibanamachine · 2020-07-15T13:33:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b2c1ee3

Build metrics

async chunks size

id	value	diff	baseline
indexManagement	2.0MB	+829.0B	2.0MB
securitySolution	9.3MB	-6.8KB	9.3MB
total	-	-6.0KB	-

miscellaneous assets size

id	value	diff	baseline
esUiShared	395.9KB	+793.0B	395.1KB
upgradeAssistant	22.5KB	-15.0B	22.6KB
total	-	+778.0B	-

page load bundle size

id	value	diff	baseline
esUiShared	1008.8KB	+2.5KB	1006.2KB

History

💔 Build #61865 failed 1910571
💛 Build #61831 was flaky fe70433
💚 Build #60847 succeeded 923471f
💔 Build #60800 failed 5fbb648
💔 Build #60530 failed 1261e01

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

stephmilovic

Case files and manual testing of the case forms all LGTM, thanks!

sebelga · 2020-07-15T14:58:42Z

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 <elasticmachine@users.noreply.github.com> Co-authored-by: Patryk Kopycinski <contact@patrykkopycinski.com>

sebelga added 3 commits July 9, 2020 14:55

Memoize form lib handlers and variables

afcc869

[MultiContent] Add getSingleContentData() handler to avoid re-renders

2063ded

Update index management forms with correct hook deps

642b84d

sebelga requested a review from a team as a code owner July 9, 2020 13:06

sebelga added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// labels Jul 9, 2020

jloleysens approved these changes Jul 9, 2020

View reviewed changes

Fix TS issues

7644ba1

alisonelizabeth approved these changes Jul 9, 2020

View reviewed changes

Merge branch 'master' into fix/form-lib-memoize

81aa8eb

Merge branch 'master' into fix/form-lib-memoize

d0506c8

Merge branch 'master' into fix/form-lib-memoize

33a2765

Merge branch 'master' into fix/form-lib-memoize

ffd2f52

Merge branch 'master' into fix/form-lib-memoize

1261e01

sebelga added 3 commits July 13, 2020 09:09

Merge remote-tracking branch 'upstream/master' into fix/form-lib-memoize

ca71048

[Security] Fix <StepAboutRuleComponent /> for memoized form

52ee926

[form_lib] Small refactor for formDefaultValue

5fbb648

sebelga requested review from a team as code owners July 13, 2020 10:58

Merge branch 'master' into fix/form-lib-memoize

923471f

yctercero reviewed Jul 13, 2020

View reviewed changes

rylnd reviewed Jul 13, 2020

View reviewed changes

sebelga added 7.10.0 and removed v7.9.0 labels Jul 14, 2020

sebelga and others added 4 commits July 15, 2020 09:14

Merge remote-tracking branch 'upstream/master' into fix/form-lib-memoize

fe70433

Remove unnecessary effect in useField()

1910571

Cleanup security_solution forms

839779b

stephmilovic approved these changes Jul 15, 2020

View reviewed changes

sebelga merged commit 99255d8 into elastic:master Jul 15, 2020

sebelga deleted the fix/form-lib-memoize branch July 15, 2020 14:58

sebelga mentioned this pull request Jul 15, 2020

[7.x] [Form lib] Memoize form hook object and fix hook array deps (#71237) #71875

Merged

stephmilovic mentioned this pull request Jul 16, 2020

[Security Solution][Case] Fix connector's dropdown with conflicting requests #72037

Merged

2 tasks

sebelga mentioned this pull request Aug 24, 2020

[Form lib] Correctly add field to form on component mount #75796

Merged

yuliacech added the v7.10.0 label Jan 6, 2022

Conversation

sebelga commented Jul 9, 2020

How to test

Uh oh!

elasticmachine commented Jul 9, 2020

Uh oh!

jloleysens left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga commented Jul 9, 2020

Uh oh!

alisonelizabeth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga commented Jul 9, 2020

Uh oh!

sebelga commented Jul 9, 2020

Uh oh!

alisonelizabeth commented Jul 9, 2020

Uh oh!

alisonelizabeth commented Jul 9, 2020

Uh oh!

alisonelizabeth commented Jul 9, 2020

Uh oh!

alisonelizabeth commented Jul 10, 2020

Uh oh!

sebelga commented Jul 13, 2020

Uh oh!

yctercero left a comment

Choose a reason for hiding this comment

Uh oh!

sebelga commented Jul 13, 2020

Uh oh!

rylnd Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

sebelga Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

rylnd Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

spong Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

sebelga Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 15, 2020

💚 Build Succeeded

Build metrics

async chunks size

miscellaneous assets size

page load bundle size

History

Uh oh!

stephmilovic left a comment

Choose a reason for hiding this comment

Uh oh!

sebelga commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jloleysens left a comment •

edited

Loading

alisonelizabeth left a comment •

edited

Loading