[embeddable] centralize should fetch embeddable observable logic #151799

nreese · 2023-02-21T22:13:00Z

Fixes #151223 and #151128

PR does the following

creates shouldFetch$ method that centralizes logic for checking when an embeddable should fetch.
updates Lens and Maps embeddable to use shouldFetch$
Adds unit tests for Maps embeddable to capture behavior of unique edge cases

…-ref HEAD~1..HEAD --fix'

…-fix'

…-ref HEAD~1..HEAD --fix'

…-fix'

nreese · 2023-02-23T18:27:17Z

@elasticmachine merge upstream

elasticmachine · 2023-02-23T18:30:01Z

Pinging @elastic/kibana-presentation (Team:Presentation)

drewdaemon

Tested Lens embeddable—works great. Thanks

ThomThomson

Code review + tested locally in Chrome with dashboard containing a map and lens embeddable. Tested that the map and lens reload when an enabled, pinned, negated etc filter is changed, that they don't reload when a disabled filter is changed, and that only the lens reloads on change of a filter controlled by the map.

Great call to centralize the should refetch observable I left a few questions and some ideas for followups to clean this even further. LGTM!

src/plugins/embeddable/public/lib/filterable_embeddable/should_fetch.tsx

x-pack/plugins/maps/public/embeddable/map_embeddable.test.tsx

ThomThomson · 2023-02-23T21:34:26Z

x-pack/plugins/maps/public/embeddable/map_embeddable.tsx

+      shouldFetch$<MapEmbeddableInput>(this.getUpdated$(), () => {
+        return {
+          ...this.getInput(),
+          filters: this._getFilters(),


It looks like these filters are only different from the ones on input due to filtering out disabled filters & those that are controlledBy this map.

It seems like disabled filters are already filtered out by onlyDisabledFiltersChanged - I wonder if we could get rid of this special treatment by adding a excludeControlledBy argument to FilterCompareOptions?

Interesting idea and something to consider for future refactors.

nreese · 2023-02-28T15:18:20Z

@elasticmachine merge upstream

kibana-ci · 2023-02-28T16:21:35Z

💚 Build Succeeded

Buildkite Build
Commit: 24a6667

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`embeddable`	83	84	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`embeddable`	430	435	+5

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id	before	after	diff
`embeddable`	8	9	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	385.3KB	385.3KB	-7.0B
`lens`	1.3MB	1.3MB	-176.0B
`maps`	2.7MB	2.7MB	-355.0B
total			-538.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`embeddable`	74.5KB	75.1KB	+548.0B

Unknown metric groups

API count

id	before	after	diff
`embeddable`	532	538	+6

ESLint disabled line counts

id	before	after	diff
`maps`	39	40	+1
`securitySolution`	428	430	+2
total			+3

Total ESLint disabled count

id	before	after	diff
`maps`	67	68	+1
`securitySolution`	506	508	+2
total			+3

History

💚 Build #110162 succeeded 3b2a94a
💚 Build #110109 succeeded 3e3e2f5
💚 Build #109874 succeeded 269918d
💔 Build #109864 failed be61f88
💔 Build #109849 failed bb3a531
💚 Build #109813 succeeded b5ab8bf

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

nickpeihl

lgtm! Great to centralize the fetching logic!

code review and tested in chrome

…stic#151799) Fixes elastic#151223 and elastic#151128 PR does the following 1) creates `shouldFetch$` method that centralizes logic for checking when an embeddable should fetch. 2) updates Lens and Maps embeddable to use `shouldFetch$` 3) Adds unit tests for Maps embeddable to capture behavior of unique edge cases --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit afb251c)

dej611 · 2023-03-07T16:52:14Z

💚 All backports created successfully

Status	Branch	Result
✅	8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

#151799) (#152838) # Backport This will backport the following commits from `main` to `8.7`: - [[embeddable] centralize should fetch embeddable observable logic (#151799)](#151799)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Nathan Reese <[email protected]>

…stic#151799) Fixes elastic#151223 and elastic#151128 PR does the following 1) creates `shouldFetch$` method that centralizes logic for checking when an embeddable should fetch. 2) updates Lens and Maps embeddable to use `shouldFetch$` 3) Adds unit tests for Maps embeddable to capture behavior of unique edge cases --------- Co-authored-by: kibanamachine <[email protected]>

nreese added 3 commits February 21, 2023 15:11

[embeddable] centralize should fetch embeddable observable logic

7432313

clean up

193fdf7

update lens embeddable to use shouldFetch$

e3af473

nreese force-pushed the issue_151223 branch from 9b5ad50 to e3af473 Compare February 21, 2023 22:27

nreese and others added 14 commits February 21, 2023 15:28

presever skip

83a37f9

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

30291bb

…-ref HEAD~1..HEAD --fix'

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

1442a93

…-fix'

tslint

b014f0f

use generics

f5ee276

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

dfd0e4a

…-ref HEAD~1..HEAD --fix'

managa shouldFetch subscription

02fdb30

fix lens jest tests

c03f3d7

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

4c6fa5e

…-ref HEAD~1..HEAD --fix'

remove empty test file

b5ab8bf

map_embeddable unit tests

bb3a531

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

be61f88

…-fix'

tslint

269918d

filters test cases

feb3a77

Merge branch 'main' into issue_151223

3e3e2f5

nreese marked this pull request as ready for review February 23, 2023 18:28

nreese requested review from a team as code owners February 23, 2023 18:28

nreese added the v8.8.0 label Feb 23, 2023

nreese added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting and removed auto-backport Deprecated - use backport:version if exact versions are needed v8.7.0 labels Feb 23, 2023

nreese requested a review from drewdaemon February 23, 2023 19:02

drewdaemon reviewed Feb 23, 2023

View reviewed changes

drewdaemon approved these changes Feb 23, 2023

View reviewed changes

ThomThomson approved these changes Feb 23, 2023

View reviewed changes

nreese added 2 commits February 23, 2023 15:03

move FilterableEmbeddableInput to types.ts

dfa8831

rename _getFilters to _getInputFilters

3b2a94a

Merge branch 'main' into issue_151223

24a6667

nickpeihl approved these changes Feb 28, 2023

View reviewed changes

nreese merged commit afb251c into elastic:main Mar 1, 2023

stratoula mentioned this pull request Mar 6, 2023

[Lens][Embeddable] Reset removable error list on search/context update #152489

Merged

9 tasks

dej611 mentioned this pull request Mar 7, 2023

[8.7] [embeddable] centralize should fetch embeddable observable logic (#151799) #152838

Merged

davismcphee mentioned this pull request Mar 23, 2023

[Lens] [Embeddable] Lens reloads twice when attributes or savedObjectId and search context change at the same time #153561

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[embeddable] centralize should fetch embeddable observable logic #151799

[embeddable] centralize should fetch embeddable observable logic #151799

nreese commented Feb 21, 2023 •

edited

Loading

nreese commented Feb 23, 2023

elasticmachine commented Feb 23, 2023

drewdaemon left a comment

ThomThomson left a comment

ThomThomson Feb 23, 2023

nreese Feb 23, 2023

nreese commented Feb 28, 2023

kibana-ci commented Feb 28, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

nickpeihl left a comment

dej611 commented Mar 7, 2023

[embeddable] centralize should fetch embeddable observable logic #151799

[embeddable] centralize should fetch embeddable observable logic #151799

Conversation

nreese commented Feb 21, 2023 • edited Loading

nreese commented Feb 23, 2023

elasticmachine commented Feb 23, 2023

drewdaemon left a comment

Choose a reason for hiding this comment

ThomThomson left a comment

Choose a reason for hiding this comment

ThomThomson Feb 23, 2023

Choose a reason for hiding this comment

nreese Feb 23, 2023

Choose a reason for hiding this comment

nreese commented Feb 28, 2023

kibana-ci commented Feb 28, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Any counts in public APIs

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

nickpeihl left a comment

Choose a reason for hiding this comment

dej611 commented Mar 7, 2023

💚 All backports created successfully

Questions ?

nreese commented Feb 21, 2023 •

edited

Loading