Implement ScopedHistory.block #91099

pgayvallet · 2021-02-11T10:33:15Z

Summary

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-02-11T14:36:37Z

Pinging @elastic/kibana-core (Team:Core)

pgayvallet · 2021-02-11T14:39:33Z

@joshdover Now that we have this, do we want to deprecates AppMountParameters.onAppLeave? I looked at the usages, and adapting all the current usages is not going to be trivial, so I'm wondering if I should deprecates this API and open a follow-up to ping the teams to migrate to History.block?

Bamieh

LGTM

afharo

Thank you for doing this. I just added a couple of NITs 😇

src/core/public/application/scoped_history.ts

test/plugin_functional/plugins/core_history_block/kibana.json

afharo · 2021-02-15T15:01:16Z

test/plugin_functional/test_suites/core_plugins/history_block.ts

+        await testSubjects.existOrFail('navigationBlockConfirmModAl');
+        await PageObjects.common.clickConfirmOnModal();
+        expect(await browser.getCurrentUrl()).to.contain('/app/core_history_block/foo');
+      });


NIT: What happens if we navigate back to core_history_block, do we get the confirmation again? Can we add a test to ensure we don't leave hanging Prompts? How about re-entering the app? Will it re-register the Prompt a 2nd time?

What happens if we navigate back to core_history_block, do we get the confirmation again?

In this specific test plugin's implementation, we don't, as we are using the Prompt component on the / page only, so the block handler will be disposed when leaving this specific page. I can add a test asserting this behavior, even if it's more the plugin's implementation detail we would be testing here.

Can we add a test to ensure we don't leave hanging Prompts? How about re-entering the app? Will it re-register the Prompt a 2nd time?

When re-entering the app, a new ScopedHistory instance will be created, and the Prompt will be reattach from this new instance to the global history. The block handlers removal when the history changes from active to inactive is more heavily tested in the ScopedHistory's unit tests (which is way easier than from FTR).

joshdover · 2021-02-16T13:10:32Z

Now that we have this, do we want to deprecates AppMountParameters.onAppLeave? I looked at the usages, and adapting all the current usages is not going to be trivial, so I'm wondering if I should deprecates this API and open a follow-up to ping the teams to migrate to History.block?

Yes, let's mark it deprecated and open a new issue for migrating existing usages.

yakhinvadim

Just tested the branch. Works perfectly in Enterprise Search!

Thanks for doing it so quickly!

cee-chen

+1-ing Vadim's approval - I assumed we were tagged in mostly for QA vs. code review, but you want a secondary reviewer as well feel free to shout (not sure how useful I'd be though :)

cee-chen · 2021-02-16T18:57:20Z

test/plugin_functional/plugins/core_history_block/kibana.json

+  "server": false,
+  "ui": true,
+  "requiredBundles": ["kibanaReact"]
+}


Just to confirm my understanding of this - if we want to use <Prompt>, do we need to include this plugin in Enterprise Search as a required dependency? Or no/is it already baked in?

I imported it from react-router-dom and it worked, so I guess it's baked in.

Feel free to correct me, if I'm wrong!

Na, everything is provided from core. I just used kibanaReact in the test plugin for some utility functions

…istory-block

kibanamachine · 2021-02-22T11:26:01Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	446	447	+1

Page load bundle

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

id	before	after	diff
`core`	478.6KB	479.8KB	+1.2KB

History

💔 Build #108274 failed d1ed614
💚 Build #105950 succeeded d4a9e69

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

* implements ScopedHistory.block * add FTR tests * fix test plugin id * update generated doc * deprecates AppMountParameters.onAppLeave * typo fix * add new FTR test * fix added test

* master: (36 commits) [Uptime] Thumbnail full screen view steps navigation fix (elastic#91895) Implement ScopedHistory.block (elastic#91099) [Lens] Fix overlowing content on a chart for charts and table (elastic#92006) handle source column differences in embeddable as well (elastic#91987) [Vega] [Map] disable map rotation using right right click / touch rotation gesture (elastic#91996) [Lens] Load indexpatterns list from indexPattern Service (elastic#91984) [coverage] ingest data in parallel (elastic#92074) [Lens] Drag and drop performance improvements (elastic#91641) A few more environment uiFilters fixes (elastic#92044) Enabling Uptime and Dashboard a11y test (elastic#91017) [Security Solution][Detections] Adds more granular validation for nested fields (elastic#92041) [Security Solution] [Detections] add overflow-wrap for description (elastic#91945) [Security Solution] [Detections] do not truncate filename in value list table in modal (elastic#91952) Skip flaky apm test elastic#91673 (elastic#92065) [docker] Default server.name to hostname (elastic#90799) Use documentation link service for snapshot restore (elastic#91596) [Security Solution] Clearing up all jest errors and warnings (elastic#91740) Add `@kbn/analytics` to UI Shared Deps (elastic#91810) [7.12][Telemetry] Add missing fields for security telemetry (elastic#91920) [Security Solution] Adds cypress-pipe (elastic#91550) ...

* implements ScopedHistory.block * add FTR tests * fix test plugin id * update generated doc * deprecates AppMountParameters.onAppLeave * typo fix * add new FTR test * fix added test

pgayvallet added 2 commits February 11, 2021 11:05

implements ScopedHistory.block

5076baf

add FTR tests

d4a9e69

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0 labels Feb 11, 2021

pgayvallet changed the title ~~Implements ScopedHistory.block~~ Implement ScopedHistory.block Feb 11, 2021

pgayvallet marked this pull request as ready for review February 11, 2021 14:36

pgayvallet requested a review from a team as a code owner February 11, 2021 14:36

pgayvallet requested review from cee-chen and joshdover February 11, 2021 14:36

Bamieh self-requested a review February 15, 2021 08:03

Bamieh approved these changes Feb 15, 2021

View reviewed changes

afharo reviewed Feb 15, 2021

View reviewed changes

yakhinvadim approved these changes Feb 16, 2021

View reviewed changes

cee-chen approved these changes Feb 16, 2021

View reviewed changes

pgayvallet added 7 commits February 22, 2021 08:21

Merge remote-tracking branch 'upstream/master' into kbn-88896-scopedh…

dbda631

…istory-block

fix test plugin id

22e3c85

update generated doc

d341f6a

deprecates AppMountParameters.onAppLeave

6c0bd0a

typo fix

d360bb5

add new FTR test

d1ed614

fix added test

6c4a505

pgayvallet merged commit eed5f72 into elastic:master Feb 22, 2021

pgayvallet mentioned this pull request Feb 22, 2021

[7.x] Implement ScopedHistory.block (#91099) #92150

Merged

pgayvallet mentioned this pull request Feb 22, 2021

Migrate usages of AppMountParameters.onAppLeave to use ScopedHistory.block instead #92187

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ScopedHistory.block #91099

Implement ScopedHistory.block #91099

pgayvallet commented Feb 11, 2021 •

edited

Loading

elasticmachine commented Feb 11, 2021

pgayvallet commented Feb 11, 2021

Bamieh left a comment

afharo left a comment •

edited

Loading

afharo Feb 15, 2021

pgayvallet Feb 22, 2021

joshdover commented Feb 16, 2021

yakhinvadim left a comment

cee-chen left a comment

cee-chen Feb 16, 2021

yakhinvadim Feb 16, 2021 •

edited

Loading

pgayvallet Feb 22, 2021

kibanamachine commented Feb 22, 2021

Implement ScopedHistory.block #91099

Implement ScopedHistory.block #91099

Conversation

pgayvallet commented Feb 11, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Feb 11, 2021

pgayvallet commented Feb 11, 2021

Bamieh left a comment

Choose a reason for hiding this comment

afharo left a comment • edited Loading

Choose a reason for hiding this comment

afharo Feb 15, 2021

Choose a reason for hiding this comment

pgayvallet Feb 22, 2021

Choose a reason for hiding this comment

joshdover commented Feb 16, 2021

yakhinvadim left a comment

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen Feb 16, 2021

Choose a reason for hiding this comment

yakhinvadim Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet Feb 22, 2021

Choose a reason for hiding this comment

kibanamachine commented Feb 22, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Page load bundle

History

pgayvallet commented Feb 11, 2021 •

edited

Loading

afharo left a comment •

edited

Loading

yakhinvadim Feb 16, 2021 •

edited

Loading