Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Enterprise Search] Update apps to use a service for docs links #89425

Merged
merged 11 commits into from
Jan 29, 2021
Merged

[Enterprise Search] Update apps to use a service for docs links #89425

merged 11 commits into from
Jan 29, 2021

Conversation

scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Jan 27, 2021 •
edited
Loading

Summary

Adds a local DocLinks service in Enterprise Search to facilitate using the Kibana DocLinksService.

I tried to hook into KibanaLogic with the data but both the routes files are instantiated before the logic mounts and this was the only way I could get the values into our apps at the start of the app lifecycle. Open to suggestions if there is a better way.

Best to follow along by commit.

Checklist

@scottybollinger scottybollinger added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Jan 27, 2021
@scottybollinger scottybollinger requested review from a team January 27, 2021 15:00
@goodroot
Copy link
Contributor

FYI @chriscressman -- this is a thing.

@scottybollinger
Copy link
Contributor Author

@elasticmachine merge upstream

yakhinvadim
yakhinvadim approved these changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@yakhinvadim yakhinvadim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks, Scotty!

@scottybollinger
Copy link
Contributor Author

Thanks @constancecchen. That was my first thought as well and I tried that. Unfortunately, the routes.ts files are instantiated before docLinksService.setDocLinks is called and the values are empty strings that are set in the constructor

image

@scottybollinger
Copy link
Contributor Author

Here's a better view:

2021-01-28_14-16-14

image

@cee-chen
Copy link
Member

My diff/commit is currently working as-is locally for me without issue. Want to hop on a screen share?

@cee-chen
Copy link
Member

cee-chen commented Jan 28, 2021 •
edited
Loading

Actually, that's super strange. It's working on http://localhost:5601/xlc/app/enterprise_search/overview/setup_guide (and all other setup guides), but it looks like you're correct that it's not working for that specific Workplace Search documentation link. Odd, looking into this now

EDIT: It's also working for http://localhost:5601/xlc/app/enterprise_search/app_search/settings/account (app search docs link). I suspect something is wrong with that specific Workplace Search doc link

@cee-chen
Copy link
Member

After some debugging w/ Scotty I've figured out that this is a very weird race condition, and sometimes does and doesn't work. e.g. App Search links were working for me initially but after some hard refreshes stopped working. So we can't rely on moving initialization to index.tsx if using this approach (static strings in routes.tsx). I'll collapse my previous comment in that case as a wontfix

cee-chen
cee-chen reviewed Jan 28, 2021
View reviewed changes
x-pack/plugins/enterprise_search/public/applications/shared/doc_links/index.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/doc_links/doc_links.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/doc_links/doc_links.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/doc_links/doc_links.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/public/applications/workplace_search/routes.ts Outdated Show resolved Hide resolved
...rch/public/applications/app_search/components/engine_overview/engine_overview_empty.test.tsx Outdated Show resolved Hide resolved
...rch/public/applications/app_search/components/engine_overview/engine_overview_empty.test.tsx Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/setup_guide/cloud/instructions.tsx Outdated Show resolved Hide resolved
...rch/public/applications/app_search/components/engine_overview/engine_overview_empty.test.tsx Outdated Show resolved Hide resolved
scottybollinger and others added 5 commits January 28, 2021 16:19
@scottybollinger
Add comment
221969f 
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@scottybollinger
Add new line
fbefb94 
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@scottybollinger
Refactor test
e6c0b68
@scottybollinger
Rename class and remove extra route segments
7e092a1
@scottybollinger
Update test names
1cb8342
cee-chen
cee-chen approved these changes Jan 28, 2021
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙇‍♀️ Thanks so much for doing this work Scotty!!

cee-chen
cee-chen reviewed Jan 28, 2021
View reviewed changes
x-pack/plugins/enterprise_search/public/applications/shared/doc_links/doc_links.ts
this.enterpriseSearchBase = docLinks.links.enterpriseSearch.base;
this.appSearchBase = docLinks.links.enterpriseSearch.appSearchBase;
this.workplaceSearchBase = docLinks.links.enterpriseSearch.workplaceSearchBase;
this.cloudBase = `${docLinks.ELASTIC_WEBSITE_URL}guide/en/cloud/current`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[not a blocker, just leaving this comment here before I forget it] It would be awesome to directly add cloudBase into the Kibana core helper someday! I'm sure other plugins might need it at some point

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1171 1165 -6

Async chunks

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

id before after diff
enterpriseSearch 1.8MB 1.8MB -34.3KB

Page load bundle

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

id before after diff
enterpriseSearch 14.4KB 15.4KB +1.0KB

History

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

@scottybollinger scottybollinger merged commit 67014a7 into elastic:master Jan 29, 2021
@scottybollinger scottybollinger deleted the scottybollinger/doc-links-service branch January 29, 2021 00:14
@chriscressman
Copy link
Contributor

This is great. Thanks for moving us over to the docs links service so quickly Scotty!

@scottybollinger scottybollinger mentioned this pull request Jan 29, 2021
Merged
scottybollinger added a commit to scottybollinger/kibana that referenced this pull request Jan 29, 2021
@scottybollinger @kibanamachine
[Enterprise Search] Update apps to use a service for docs links (elas…
6d4b673 
…tic#89425)

* Create DocLinksService

* Set docLinks on app start

* Update routes modules to use service

* Update component and test to use service

* Remove legacy files

* Add comment

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

* Add new line

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

* Refactor test

* Rename class and remove extra route segments

* Update test names

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
scottybollinger added a commit that referenced this pull request Jan 30, 2021
@scottybollinger @kibanamachine
[Enterprise Search] Update apps to use a service for docs links (#89425…
6bb6afd 
…) (#89785)

* Create DocLinksService

* Set docLinks on app start

* Update routes modules to use service

* Update component and test to use service

* Remove legacy files

* Add comment

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

* Add new line

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

* Refactor test

* Rename class and remove extra route segments

* Update test names

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Constance <constancecchen@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

@yakhinvadim yakhinvadim yakhinvadim approved these changes

Assignees
No one assigned
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@scottybollinger @goodroot @cee-chen @kibanamachine @chriscressman @yakhinvadim