Use documentation links service provided by the core directly. #88158

azasypkin · 2021-01-13T11:39:49Z

Summary

Since core now provides all the necessary links directly there is no reason to build our own abstractions around DocLinks service. Less code === less bugs and it's also easier to find all places that use a particular link, just in case.

cc @lcawl

azasypkin · 2021-01-13T11:41:04Z

src/core/public/doc_links/doc_links_service.ts

@@ -220,7 +220,7 @@ export class DocLinksService {
        },
        apis: {
          createIndex: `${ELASTICSEARCH_DOCS}indices-create-index.html`,
-          createSnapshotLifecylePolicy: `${ELASTICSEARCH_DOCS}slm-api-put-policy.html`,
+          createSnapshotLifecyclePolicy: `${ELASTICSEARCH_DOCS}slm-api-put-policy.html`,


note: looks like nobody is using this yet.

azasypkin · 2021-01-13T11:42:37Z

src/core/public/doc_links/doc_links_service.ts

@@ -344,12 +344,38 @@ export interface DocLinksStart {
    readonly ml: Record<string, string>;
    readonly transforms: Record<string, string>;
    readonly visualize: Record<string, string>;
-    readonly apis: Record<string, string>;
+    readonly apis: {


note: it's very easy to make a typo in a link name and with Record<string, string> signature it may go unnoticed. Changing signatures only for those that we use in Security for now.

This is much better! Makes it easier to know what links are available as well with autocomplete.

The properties you've added don't have the readonly flag. Might be worth just wrapping the whole interface in Readonly<T> utility type since literally everything should be marked as readonly.
https://www.typescriptlang.org/docs/handbook/utility-types.html#readonlytype

Ah, good catch! Will do

elasticmachine · 2021-01-13T14:22:33Z

Pinging @elastic/kibana-security (Team:Security)

thomheymann · 2021-01-14T09:35:03Z

Ah nice, I didn't know we were able to extend the links available through DocLinksService.

I have created a DocLink component (and hook) that uses that service and allows developers to create links to documentation: https://github.com/elastic/kibana/blob/99906a0b08e5ad77f8d04fb1eb338d1b7ede8036/x-pack/plugins/security/public/components/doc_link.tsx

I am trying to move us away from having to manually pass down global services like this through our render tree.

Could be useful for this PR as well and includes the correct styling for external links.

Component

I personally prefer linking directly, instead of maintaining a list of all possible links via doc links service. These links are not dynamic and hardly ever change so having that indirection doesn't really solve anything that a simple search-replace can't handle. It also means that it's very easy to end up with unused URLs in the service when the link is removed from our components but the service still has them registered. Having said that, using predefined links like you do is possible via the useDocLinks hook (See Hook example).

<DocLink app="elasticsearch" doc="built-in-roles.html">
  Learn what privileges individual roles grant.
</DocLink>

Hook

const [docs] = useDocLinks();

<EuiLink href={docs.dashboard.guide} target="_blank" external>
  Learn how to get started with dashboards.
</EuiLink>

azasypkin · 2021-01-14T10:14:17Z

It also means that it's very easy to end up with unused URLs in the service when the link is removed from our components but the service still has them registered.

Yes, it's a valid point. But I can imagine that having those in a single place for such a large product can make life of our Docs team easier: same approach across all plugins + search-replace may not catch typos in attributes like these doc=buitl-ni-rols.htlm".

Having said that, using predefined links like you do is possible via the useDocLinks hook (See Hook example).

Awesome, didn't see this one! Let me check if we have hooks-ready components where I can use useDocLinks.

thomheymann · 2021-01-14T14:05:27Z

src/core/public/doc_links/doc_links_service.ts

@@ -344,12 +344,38 @@ export interface DocLinksStart {
    readonly ml: Record<string, string>;
    readonly transforms: Record<string, string>;
    readonly visualize: Record<string, string>;
-    readonly apis: Record<string, string>;
+    readonly apis: {


This is much better! Makes it easier to know what links are available as well with autocomplete.

The properties you've added don't have the readonly flag. Might be worth just wrapping the whole interface in Readonly<T> utility type since literally everything should be marked as readonly.
https://www.typescriptlang.org/docs/handbook/utility-types.html#readonlytype

thomheymann · 2021-01-14T14:08:02Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.test.tsx

-      apiKeysAPIClient: apiClientMock,
-    };
+    return mountWithIntl(
+      <KibanaContextProvider services={{ application, docLinks }}>


I would pass in the entire coreStart object into the kibana context provider here so that we don't get errors when a child component starts using e.g. notification or http service.

I usually like to pass only what we use that makes it much easier to analyze affected code if we migrate/deprecate/remove anything (and we do this constantly). But in case of Core services I agree that's handy and tolerable, will change.

thomheymann · 2021-01-14T14:09:42Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/empty_prompt/empty_prompt.tsx

+              defaultMessage="You can create an {link} from Console."
+              values={{
+                link: (
+                  <EuiLink href={`${docLinks.links.apis.createApiKey}`} target="_blank">


nit: Shouldn't the doc link be a string in itself? I don't think we need to wrap it in template literals

It should, just kept it as it was before - will fix.

thomheymann · 2021-01-14T14:11:01Z

x-pack/plugins/security/public/management/api_keys/api_keys_management_app.tsx

-              apiKeysAPIClient={new APIKeysAPIClient(http)}
-            />
-          </i18nStart.Context>,
+          <KibanaContextProvider services={{ application, docLinks }}>


Same as other context provider, it would be safer to pass through the entire coreStart object.

thomheymann · 2021-01-14T14:11:23Z

x-pack/plugins/security/public/management/api_keys/api_keys_management_app.tsx

-            <APIKeysGridPage
-              navigateToApp={application.navigateToApp}
-              notifications={notifications}
-              docLinks={new DocumentationLinksService(docLinks)}


Yesss! 🙏

thomheymann · 2021-01-14T14:12:25Z

...ity/public/management/role_mappings/components/no_compatible_realms/no_compatible_realms.tsx

+        defaultMessage="Role mappings will not be applied to any users. Contact your system administrator and refer to the {link} for more information."
+        values={{
+          link: (
+            <EuiLink href={docLinks.links.security.mappingRoles} external={true} target="_blank">


nit: boolean props don't require ={true}

lcawl · 2021-01-14T16:48:22Z

... I can imagine that having those in a single place for such a large product can make life of our Docs team easier: same approach across all plugins + search-replace may not catch typos in attributes like these doc=buitl-ni-rols.htlm".

Yes, we're hoping that using the doc link service will make it easier for us to keep links up-to-date as content moves and changes. We've found quite a few broken links so far sprinkled across the code, so it doesn't seem like there is distributed link-checking. Whereas we're working on getting all the links in the service automatically checked.

kibanamachine · 2021-01-15T10:04:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 778def9

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`security`	472	468	-4

Async chunks

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

id	before	after	diff
`security`	786.5KB	786.9KB	+374.0B

Page load bundle

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

id	before	after	diff
`core`	564.6KB	564.6KB	+1.0B
`security`	164.9KB	161.5KB	-3.5KB
total			-3.5KB

History

💚 Build #98841 succeeded d2952c3
💚 Build #98520 succeeded 9cca1c8

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

thomheymann

LGTM! 🎉

…ic#88158)

azasypkin · 2021-01-15T16:14:09Z

7.x/7.12.0: 69e3111

azasypkin added chore Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Jan 13, 2021

azasypkin commented Jan 13, 2021

View reviewed changes

Use documentation links service provided by the core directly.

9cca1c8

azasypkin force-pushed the issue-xxx-doc-services-clean branch from b885ec5 to 9cca1c8 Compare January 13, 2021 11:49

azasypkin requested review from a team as code owners January 13, 2021 14:22

azasypkin marked this pull request as ready for review January 13, 2021 14:22

lcawl mentioned this pull request Jan 13, 2021

Move from hard-coded links to documentation link service #88107

Closed

mshustov approved these changes Jan 14, 2021

View reviewed changes

azasypkin added 2 commits January 14, 2021 11:56

Merge branch 'master' into issue-xxx-doc-services-clean

6638f1b

Review#1: use useKibana hook.

d2952c3

thomheymann reviewed Jan 14, 2021

View reviewed changes

azasypkin added 2 commits January 15, 2021 07:59

Merge branch 'master' into issue-xxx-doc-services-clean

ea1cae6

Review#2: handle review suggestions.

778def9

azasypkin requested a review from thomheymann January 15, 2021 09:44

thomheymann approved these changes Jan 15, 2021

View reviewed changes

azasypkin merged commit abfd8bb into elastic:master Jan 15, 2021

azasypkin deleted the issue-xxx-doc-services-clean branch January 15, 2021 12:31

azasypkin mentioned this pull request Jan 15, 2021

[7.x] Use documentation links service provided by the core directly. (#88158) #88462

Merged

azasypkin added a commit to azasypkin/kibana that referenced this pull request Jan 15, 2021

Use documentation links service provided by the core directly. (elast…

8895953

…ic#88158)

azasypkin added the backported label Jan 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use documentation links service provided by the core directly. #88158

Use documentation links service provided by the core directly. #88158

azasypkin commented Jan 13, 2021 •

edited

Loading

azasypkin Jan 13, 2021

azasypkin Jan 13, 2021

thomheymann Jan 14, 2021

azasypkin Jan 15, 2021

elasticmachine commented Jan 13, 2021

thomheymann commented Jan 14, 2021 •

edited

Loading

azasypkin commented Jan 14, 2021

thomheymann Jan 14, 2021

thomheymann Jan 14, 2021

azasypkin Jan 15, 2021

thomheymann Jan 14, 2021

azasypkin Jan 15, 2021

thomheymann Jan 14, 2021

thomheymann Jan 14, 2021

thomheymann Jan 14, 2021

lcawl commented Jan 14, 2021

kibanamachine commented Jan 15, 2021

thomheymann left a comment

azasypkin commented Jan 15, 2021

Use documentation links service provided by the core directly. #88158

Use documentation links service provided by the core directly. #88158

Conversation

azasypkin commented Jan 13, 2021 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jan 13, 2021

thomheymann commented Jan 14, 2021 • edited Loading

Component

Hook

azasypkin commented Jan 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcawl commented Jan 14, 2021

kibanamachine commented Jan 15, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

thomheymann left a comment

Choose a reason for hiding this comment

azasypkin commented Jan 15, 2021

azasypkin commented Jan 13, 2021 •

edited

Loading

thomheymann commented Jan 14, 2021 •

edited

Loading