Skip to content

[Canvas] Only fetch saved elements once#71310

Merged
crob611 merged 2 commits intoelastic:masterfrom
crob611:bug/saved-elements
Jul 14, 2020
Merged

[Canvas] Only fetch saved elements once#71310
crob611 merged 2 commits intoelastic:masterfrom
crob611:bug/saved-elements

Conversation

@crob611
Copy link
Copy Markdown
Contributor

@crob611 crob611 commented Jul 9, 2020

Summary

Saw this bug where there was no requirements on a useEffect so when you went to add a Custom Element, it would repeatedly request the custom elements.

This adds requirements to the useEffect and adds a ref that gets flipped because the findCustomElements function is a new function on every rerender 😦

@crob611 crob611 requested a review from a team as a code owner July 9, 2020 20:03
@crob611 crob611 added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v7.8.2 v7.9.0 v8.0.0 labels Jul 9, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 9, 2020

Pinging @elastic/kibana-canvas (Team:Canvas)

@crob611 crob611 added release_note:skip Skip the PR/issue when compiling release notes review loe:small Small Level of Effort labels Jul 9, 2020
@clintandrewhall
Copy link
Copy Markdown
Contributor

clintandrewhall commented Jul 9, 2020 •
edited
Loading

Nice catch, but the bug is that useEffect should have an empty array as a dependency list, rather than no list at all:

If you want to run an effect and clean it up only once (on mount and unmount), you can pass an empty array ([]) as a second argument. This tells React that your effect doesn’t depend on any values from props or state, so it never needs to re-run. This isn’t handled as a special case — it follows directly from how the dependencies array always works.

As such, you don't need the ref.

https://reactjs.org/docs/hooks-effect.html

EDIT: I'm betting this is going to trigger the exhausting-deps rule, though... which is probably why this is missing that array in the first place. :-/

@crob611
Copy link
Copy Markdown
Contributor Author

crob611 commented Jul 13, 2020

@clintandrewhall Yep, the exhaustive-deps led me to the ref way of accomplishing this.

Should we go forward with this way, or intentionally disable exhaustive-deps and use the empty array to get the functionality we actually want?

@clintandrewhall
Copy link
Copy Markdown
Contributor

clintandrewhall commented Jul 13, 2020

Let's go with this for now... we need the bug fix. Let's look into some better ways to do this/refactor the props to not need a function/etc for the next release.

@crob611
Copy link
Copy Markdown
Contributor Author

crob611 commented Jul 13, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Jul 13, 2020

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@crob611 crob611 merged commit 1ceaea1 into elastic:master Jul 14, 2020
@crob611 crob611 mentioned this pull request Jul 14, 2020
Merged
crob611 pushed a commit to crob611/kibana that referenced this pull request Jul 14, 2020
@elasticmachine
Only fetch saved elements once (elastic#71310)
f16c3a0 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@crob611 crob611 mentioned this pull request Jul 14, 2020
Merged
crob611 pushed a commit to crob611/kibana that referenced this pull request Jul 14, 2020
@elasticmachine
Only fetch saved elements once (elastic#71310)
4e2fd30 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
crob611 pushed a commit that referenced this pull request Jul 14, 2020
@elasticmachine
Only fetch saved elements once (#71310) (#71636)
a08683c 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
crob611 pushed a commit that referenced this pull request Jul 14, 2020
@elasticmachine
Only fetch saved elements once (#71310) (#71635)
703d685 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 14, 2020
@gmmorris
Merge branch 'master' into alerting/consumer-based-rbac
97b4262 
* master: (21 commits)
  [Maps] 7.9 design improvements (elastic#71563)
  [ML] Changing all calls to ML endpoints to use internal user (elastic#70487)
  [eventLog] prevent log writing when initialization fails (elastic#71339)
  [Observability] landing page always being displayed (elastic#71494)
  [IM] Address data stream copy feedback (elastic#71615)
  [Logs UI] Anomalies page dataset filtering (elastic#71110)
  [data.search.aggs] Remove `use_field_mapping` from top hits agg (elastic#71168)
  [ML] Anomaly swim lane embeddable navigation and filter actions (elastic#71082)
  Fixes typo in siem_cloudtrail job description (elastic#71569)
  Require granted API Keys to have a name (elastic#71623)
  Update  getUsageForCollection (elastic#71609)
  Only fetch saved elements once (elastic#71310)
  [SecuritySolution][Resolver] Adding siem index and guarding process ancestry (elastic#71570)
  [APM] Additional data telemetry changes (elastic#71112)
  [Visualize] Fix export table for table export links (elastic#71249)
  [Search] Server side search API (elastic#70446)
  use inclusive language (elastic#71607)
  [Security Solution] Hide timeline footer when Resolver is open (elastic#71516)
  [Index template wizard] Remove shadow and use border for components panels (elastic#71606)
  [ML] Kibana API endpoint for histogram chart data (elastic#70976)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clintandrewhall clintandrewhall clintandrewhall approved these changes

Assignees

No one assigned

Labels

loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v7.8.2 v7.9.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@crob611 @elasticmachine @clintandrewhall @kibanamachine