[ML] Improvements for urlState hook. #70576

walterra · 2020-07-02T10:43:09Z

Summary

Makes two improvements to the urlState hook (also known as appState in some places):

There was always a risk to run into a race condition because setUrlState could refer to a stale version of the state to act upon, for example if two calls were done in parallel. This is now fixed by using a local state copy of what we get from useLocation(). This allows us to use the callback version of useState's set function so we can make sure we always modify the latest state.
Calls to history.push() are now gated by a check if the change actually referred to the corresponding instance of urlState (either _g or _a), this should reduce the updates resulting re-renders.

The two changes should make the use of setUrlState more safe against the pitfalls (race conditions/stale updates/lots of rerenders) we previously faced.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-07-02T10:43:11Z

Pinging @elastic/ml-ui (:ml)

darnautov · 2020-07-02T10:55:18Z

x-pack/plugins/ml/public/application/util/url_state.ts

+    // Only push to history if something related to the accessor of this
+    // url state instance is affected (e.g. a change in '_g' should not trigger
+    // a push in the '_a' instance).
+    if (!isEqual(getUrlState(locationSearch)[accessor], getUrlState(search)[accessor])) {


I'd suggest storing the result of getUrlState in some state value or observable and update it based on URL changes. It will let use to avoid retrieving the value from URL string on each render.

Maybe the naming was just not as obvious, but that PR already does that: locationSearch refers to what we get from React Router's useLocation, search is the state value we keep. getUrlState() is a pure function that just parses the search string so we can compare the namespace/accessor (_g/_a), it's not touching the URL directly.

I pushed some more commits with renames to make this more obvious:

renamed getUrlState to parseUrlState

renamed locationSearch to locationSearchString

renamed search to searchString

Also added a useMemo at the end of the hook so we return a memoized version of the parsed url state.

yes, this commit 06b0235 introduced useMemo with a parsed object, this is what I meant 👍

darnautov · 2020-07-02T13:31:27Z

x-pack/plugins/ml/public/application/util/url_state.ts

  );

-  return [getUrlState(search)[accessor], setUrlState];
+  const urlState = useMemo(() => parseUrlState(searchString)[accessor], [searchString]);


I think it worth to mention in a comment for this hook that it does not react on the accessor parameter update

Good point, the accessor should never be changed, I updated the code to enforce that so it only ever considers the value passed on first call: 3500f11#diff-42f8ed66c3f1b64589d16f1b4c9d1aa2R63-R65

darnautov

LGTM! Great enhancement

darnautov · 2020-07-02T13:41:03Z

x-pack/plugins/ml/public/application/util/url_state.ts

+    // url state instance is affected (e.g. a change in '_g' should not trigger
+    // a push in the '_a' instance).
+    if (
+      !isEqual(parseUrlState(locationSearchString)[accessor], parseUrlState(searchString)[accessor])


looks like this effect should depend on two variables, you are missing locationSearchString

I added more comments in 3500f11 to explain why locationSearchString isn't necessary for comparison in this case.

peteharverson

Tested and LGTM

alvarezmelissa87

LGTM ⚡

walterra · 2020-07-03T07:39:44Z

@elasticmachine merge upstream

walterra · 2020-07-06T08:40:21Z

@darnautov @alvarezmelissa87 @peteharverson

The functional tests surfaced that there were still issues with possible race conditions and render loops.
The problem was that multiple instances of useUrlState would manage their own state and could end up with different stale state when doing an update.
I solved this now by moving the logic to update the params string to a React context so there is ever only one instance of the code that manages url state updates. The hook useUrlState uses that context and keeps the same API surface, so the overall code in components can stay the same. The only difference is that I added the corresponding provider to the root of the ML plugin.

In the long run we should try to migrate away from HashRouter to BrowserRouter, this would simplify how we can manage this because it has better support for url params.

alvarezmelissa87

Code LGTM ⚡

peteharverson

Tested latest edits and LGTM

walterra · 2020-07-08T13:10:20Z

@elasticmachine merge upstream

walterra · 2020-07-08T15:11:49Z

@elasticmachine merge upstream

walterra · 2020-07-08T18:34:33Z

@elasticmachine merge upstream

kibanamachine · 2020-07-08T20:04:02Z

💚 Build Succeeded

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

Build metrics

✅ unchanged

History

💛 Build #59791 was flaky d39d470
💛 Build #59744 was flaky a4933fe
💚 Build #58921 succeeded 33fed07
💔 Build #58881 failed 53d20e6
💔 Build #58838 failed a80e5c8

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

Makes two improvements to the urlState hook (also known as appState in some places): - There was always a risk to run into a race condition because setUrlState could refer to a stale version of the state to act upon, for example if two calls were done in parallel. This is now fixed by using a local state copy of what we get from useLocation(). This allows us to use the callback version of useState's set function so we can make sure we always modify the latest state. - Calls to history.push() are now gated by a check if the change actually referred to the corresponding instance of urlState (either _g or _a), this should reduce the updates resulting re-renders. The two changes should make the use of setUrlState more safe against the pitfalls (race conditions/stale updates/lots of rerenders) we previously faced.

* master: (39 commits) [APM] Add warning to notify user about legacy ML jobs (elastic#71030) updates consumer to siem (elastic#71117) Index pattern creation flow - fix spelling (elastic#71192) [Security Solution][Endpoint] User Manifest Cleanup + Artifact Compression (elastic#70759) [SECURITY] Rearrange rule name's column in Alert Table (elastic#71020) [SECURITY] Alerts back to Detections (elastic#71142) [Security Solution][Exceptions Builder] - Fixes operator selection bug (elastic#71178) [SIEM][Detection Engine] Speeds up value list imports by enabling streaming of files. [APM] Update ML job ID in data telemetry tasks (elastic#71044) [Resolver] Remove `currentPanelView` selector (elastic#71154) add meta.managed to index templates (elastic#71135) Clarify trial subscription levels (elastic#70900) [Security Solution] fix panel links (elastic#71148) skip flaky suite (elastic#69632) skip suite failing ES Promotion (elastic#71018) [ML] DF Analytics: add results field to wizard and show regression stats (elastic#70893) [SIEM] update wordings (elastic#71119) [SECURITY SOLUTION] Rename to hosts and administration (elastic#70913) [ML] Improvements for urlState hook. (elastic#70576) Removing uptime guide (elastic#71124) ...

Makes two improvements to the urlState hook (also known as appState in some places): - There was always a risk to run into a race condition because setUrlState could refer to a stale version of the state to act upon, for example if two calls were done in parallel. This is now fixed by using a local state copy of what we get from useLocation(). This allows us to use the callback version of useState's set function so we can make sure we always modify the latest state. - Calls to history.push() are now gated by a check if the change actually referred to the corresponding instance of urlState (either _g or _a), this should reduce the updates resulting re-renders. The two changes should make the use of setUrlState more safe against the pitfalls (race conditions/stale updates/lots of rerenders) we previously faced. Co-authored-by: Elastic Machine <[email protected]>

[ML] Fix/improve url state.

239f510

walterra added :ml refactoring v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 2, 2020

walterra requested a review from a team as a code owner July 2, 2020 10:43

walterra self-assigned this Jul 2, 2020

walterra requested review from darnautov, peteharverson and alvarezmelissa87 July 2, 2020 10:43

darnautov reviewed Jul 2, 2020

View reviewed changes

walterra added 4 commits July 2, 2020 13:08

[ML] Renames for clarity.

06b0235

[ML] More renames.

9c9686a

Merge branch 'master' into ml-improve-url-state

4bbf4f0

[ML] Adjust tests.

4b43b48

darnautov reviewed Jul 2, 2020

View reviewed changes

darnautov approved these changes Jul 2, 2020

View reviewed changes

darnautov reviewed Jul 2, 2020

View reviewed changes

peteharverson approved these changes Jul 2, 2020

View reviewed changes

alvarezmelissa87 approved these changes Jul 2, 2020

View reviewed changes

[ML] Enforce to use only accessor default value. More comments.

3500f11

elasticmachine and others added 4 commits July 3, 2020 01:39

Merge branch 'master' into ml-improve-url-state

1b7eda2

[ML] Use context to have only one instance of url state.

9993a13

[ML] Fix TS and unit tests.

1e8514b

[ML] Reduce rerenders.

60911c3

Merge branch 'master' into ml-improve-url-state

a80e5c8

walterra requested review from darnautov, alvarezmelissa87 and peteharverson July 6, 2020 08:41

walterra added 2 commits July 6, 2020 12:36

[ML] Update tests to include UrlStateProvider.

53d20e6

Merge branch 'master' into ml-improve-url-state

33fed07

alvarezmelissa87 approved these changes Jul 6, 2020

View reviewed changes

peteharverson approved these changes Jul 7, 2020

View reviewed changes

Merge branch 'master' into ml-improve-url-state

a4933fe

Merge branch 'master' into ml-improve-url-state

d39d470

Merge branch 'master' into ml-improve-url-state

ad2788f

walterra merged commit 91377b2 into elastic:master Jul 8, 2020

walterra deleted the ml-improve-url-state branch July 8, 2020 20:58

walterra mentioned this pull request Jul 8, 2020

[7.x] [ML] Improvements for urlState hook. (#70576) #71156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Improvements for urlState hook. #70576

[ML] Improvements for urlState hook. #70576

walterra commented Jul 2, 2020 •

edited

Loading

elasticmachine commented Jul 2, 2020

darnautov Jul 2, 2020

walterra Jul 2, 2020

darnautov Jul 2, 2020

darnautov Jul 2, 2020

walterra Jul 2, 2020

darnautov left a comment

darnautov Jul 2, 2020

walterra Jul 2, 2020

peteharverson left a comment

alvarezmelissa87 left a comment

walterra commented Jul 3, 2020

walterra commented Jul 6, 2020

alvarezmelissa87 left a comment

peteharverson left a comment

walterra commented Jul 8, 2020

walterra commented Jul 8, 2020

walterra commented Jul 8, 2020

kibanamachine commented Jul 8, 2020

[ML] Improvements for urlState hook. #70576

[ML] Improvements for urlState hook. #70576

Conversation

walterra commented Jul 2, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jul 2, 2020

darnautov Jul 2, 2020

Choose a reason for hiding this comment

walterra Jul 2, 2020

Choose a reason for hiding this comment

darnautov Jul 2, 2020

Choose a reason for hiding this comment

darnautov Jul 2, 2020

Choose a reason for hiding this comment

walterra Jul 2, 2020

Choose a reason for hiding this comment

darnautov left a comment

Choose a reason for hiding this comment

darnautov Jul 2, 2020

Choose a reason for hiding this comment

walterra Jul 2, 2020

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

walterra commented Jul 3, 2020

walterra commented Jul 6, 2020

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

walterra commented Jul 8, 2020

walterra commented Jul 8, 2020

walterra commented Jul 8, 2020

kibanamachine commented Jul 8, 2020

💚 Build Succeeded

Build metrics

History

walterra commented Jul 2, 2020 •

edited

Loading