add RedirectAppLinks component in kibana_react #67595

pgayvallet · 2020-05-28T10:27:36Z

Summary

Add a <RedirectAppLinks> component in kibana_react to help with cross-app linking.

The component will intercept any click performed on <a> children and call application.navigateToUrl to avoid performing a full page refresh when the link points to a valid kibana application route.

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 · 2020-05-28T11:03:40Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2020-05-28T15:12:20Z

src/core/public/application/application_service.tsx

@@ -306,6 +306,7 @@ export class ApplicationService {
        const relUrl = http.basePath.prepend(getAppUrl(availableMounters, appId, path));
        return absolute ? relativeToAbsolute(relUrl) : relUrl;
      },
+      parseAppUrl: (url) => parseAppUrl(url, http.basePath, this.apps),


As the new RedirectCrossAppLinks component should ignore links pointing to the current application, I couldn't just use navigateToApp directly, and had to expose parseAppUrl to allow the consumer to check if the app from the link is, or not, the active app.

Exposing this API still makes sense anyway I think

pgayvallet · 2020-05-28T15:15:55Z

src/plugins/kibana_react/public/app_links/redirect_cross_app_link.test.tsx

+  it('intercept click events on children link elements', () => {
+    let event: MouseEvent;
+
+    const component = mount(
+      <div
+        onClick={(e) => {
+          event = e;
+        }}
+      >


I feel like these enzyme based tests are as good as actual FTR test, as the only difference is that they are using jsdoc instead of the actual browser implementation. It also allow more precise testing, as we can collect the event to perform verifications, which would not be possible on FTR tests.

Tell me if you think that adding FTR tests is still necessary.

Nope. Seems fine to me. Can be covered by FTR when in use by actual applications.

…t-crossapp-links

joshdover

Is there any UI we could drop this into now for testing? Maybe the SavedObjects UI?

src/plugins/kibana_react/public/app_links/utils.ts

joshdover · 2020-05-28T17:36:08Z

src/plugins/kibana_react/public/app_links/utils.test.ts

+
+import { getClosestLink } from './utils';
+
+const createBranch = (...tags: string[]): HTMLElement[] => {


It might be simpler to just use raw html strings. I prefer this because it eliminates the possibility of there being a bug in this setup code.

const element = document.createElement('div') element.innerHTML = `<div><a></a></div>`;

This utility method has the advantage to return the created branch elements as an array

const [, container, target] = createBranch('A', 'DIV', 'SPAN');

Using plain innerHTML would force to manually access each individual node of the branch in the tests. I think the decreased readability is a biggest downside than a bug risk in the createBranch method?

I could also change to something like

const element = document.createElement('a') element.innerHTML = `<div><span></span></div>`; const [, container, target] = getLeftmostBranch(element);

but it's still 3 lines versus one (and getLeftmostBranch would have a logic very close to createBranch anyway)

wdyt?

joshdover · 2020-05-28T17:40:36Z

src/plugins/kibana_react/public/app_links/click_handler.ts

+      !hasActiveModifierKey(e) // ignore clicks with modifier keys
+    ) {
+      const appInfo = parseAppUrl(link.href);
+      if (appInfo && appInfo.app !== currentAppId) {


Remind me again why we can't call this for links within the same app?

It'd be nice if this solution worked for all links

It would work for intra-app links. It's just an opinionated decision, based on the fact that most (all) out apps are currently using react-router and the Link component. Handling links to the active link could/would result in a click on the link be handled both by the Link click handler and ours, resulting on performing the history.push twice. (Also the name of our component is redirect**Cross**AppLinks

But I'm unsure about that TBH, maybe we should just decide than any app links should be handled and document that using Link inside RedirectCrossAppLinks should be avoided? wdyt @joshdover @Dosant ?

Looking at https://github.com/ReactTraining/react-router/blob/master/packages/react-router-dom/modules/Link.js#L49, the Link onClick does prevent default, so clicking on a Link inside our component would juste trigger the Link's navigation, and not ours.

We are probably fine to remove the currentApp check then. Which also means we can use the component for the navlinks.

Should the component be renamed to RedirectAppLinks then?

based on the fact that most (all) out apps are currently using react-router and the Link component

I'm not sure this is true? In my rudimentary search (git grep "import.*Link.*from 'react-router*" -- {src,x-pack}/plugins) I only found that beats_management and uptime plugins were using this component. I think the main reason devs are not using this component is that you can't really use it with EuiButton or EuiLink.

We are probably fine to remove the currentApp check then. Which also means we can use the component for the navlinks.

Should the component be renamed to RedirectAppLinks then?

If it works, than a rename makes sense to me 👍

src/plugins/kibana_react/public/app_links/click_handler.ts

…t-crossapp-links

pgayvallet · 2020-06-02T07:17:22Z

Is there any UI we could drop this into now for testing? Maybe the SavedObjects UI?

I added it to the SOM table, however as the management plugin is still in legacy mode (the KP management plugin is still just a bridge to the legacy plugin), all links are still triggering full page refresh (however with actual debugging I saw that it worked, but it's not enough).

Not sure where we could wire this for testing. @Dosant maybe you have an idea?

Dosant · 2020-06-02T10:00:43Z

@pgayvallet

I added it to the SOM table, however as the management plugin is still in legacy mode (the KP management plugin is still just a bridge to the legacy plugin), all links are still triggering full page refresh (however with actual debugging I saw that it worked, but it's not enough).
Not sure where we could wire this for testing. @Dosant maybe you have an idea?

How about our examples? https://github.com/elastic/kibana/tree/master/examples
There are 3 state management demo apps and we could try to link them together?
Like a footer in the bottom of each demo app with links to other state management demo apps?

There are also functional tests for those demos, so we could cover that navigation in function test

to run with examples use: yarn start --run-examples

pgayvallet · 2020-06-02T14:40:55Z

Is there any UI we could drop this into now for testing? Maybe the SavedObjects UI?

Remind me again why we can't call this for links within the same app?

Another option, if we get rid of this non-same-app check: We could also wire it directly to the nav by removing the current custom onClick handler in src/core/public/chrome/ui/header/nav_link.tsx

…t-crossapp-links

pgayvallet · 2020-06-03T09:50:10Z

Adapted and renamed the component to no longer exclude the current app.

We could also wire it directly to the nav by removing the current custom onClick handler in src/core/public/chrome/ui/header/nav_link.tsx

Actually we can't, as the component lives in the kibana_react plugin and the header in core, and we can't import plugin code in core.

…t-crossapp-links

pgayvallet · 2020-06-04T11:34:06Z

Due to the lack of any obvious place to do it, I ended up adding FTR tests instead of wiring the feature to some existing links.

PR should be in its final state now.

mshustov · 2020-06-04T12:41:29Z

src/plugins/kibana_react/public/app_links/click_handler.ts

+    // see https://github.com/DefinitelyTyped/DefinitelyTyped/pull/12239
+    const target = e.target as HTMLElement;
+
+    const link = getClosestLink(target, container);


Could use built-in Element.closest() method?

Suggested change

const link = getClosestLink(target, container);

const link = target.closest('.containerClass a');

Not sure, due to browser support: https://caniuse.com/#search=closest?
Unfortunately I don't have a browserstack account to ensure this API is properly shimmed by corejs

Do we have a usable account somewhere?

We are discounting IE11 support in v7.9, so it's not a problem.

That requires quite a bit of changes in the handler unit tests as it can no longer pass down the container for testing, but I guess using native APIs should be encouraged.

Hum. it seems that our version of jsdom doesnt supports closest, which is strange as it's supposed to be available for quite a long time now.

TypeError: target.closest is not a function 41 | const target = e.target as HTMLElement; 42 | > 43 | const link = target.closest(`.${containerClass} a`) as HTMLAnchorElement;

console.log(target, target.parentElement, target.closest, target.parentElement.closest) // HTMLAnchorElement {} HTMLDivElement {...} undefined undefined

Confirmed: the jsdom version that (our version of) jest is using (node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/nodes/Element-impl.js) is a 11.5.1, which does not implements closest...

Latest jest does have a version that implements it though (https://github.com/facebook/jest/blob/master/packages/jest-environment-jsdom/package.json)

That's a shame. Let's keep the current implementation then. We can add a comment to switch to native API later.

Will add a comment for when #58095 lands.

Unfortunately, I wasn't able to bring the most recent version of jsdom in that PR, because seems a combination of jest@25 with jsdom>=14 got serious performance degradation and we cannot bump to jest@26, because it requires typescript>=3.8, so I hope @restrry PR gets merged soon and I will give it a try to jest@26 and jsdom@16

mshustov · 2020-06-04T12:46:18Z

src/plugins/kibana_react/public/app_links/click_handler.ts

+  navigateToUrl: ApplicationStart['navigateToUrl'];
+}
+
+export const createCrossAppClickHandler = ({


This method doesn't contain any logic to enforce the cross-app navigation. Does it?

good catch, this one wasn't properly renamed when we decided to get rid of the cross-app limitation.

mshustov · 2020-06-04T13:03:10Z

src/plugins/kibana_react/public/app_links/redirect_app_link.tsx

+ * Utility component that will intercept click events on children anchor (`<a>`) elements to call
+ * `application.navigateToUrl` with the link's href. This will trigger SPA friendly navigation
+ * when the link points to a valid kibana app.
+ *


Should we recommend using it on the topmost level of the component tree?

mshustov · 2020-06-04T13:14:03Z

test/plugin_functional/test_suites/application_links/redirect_app_links.ts

+  decodeURIComponent(
+    url.format({
+      protocol: 'http:',
+      hostname: process.env.TEST_KIBANA_HOST || 'localhost',


Optional: our tests shouldn't rely on the test environment too much since we do not have control over it. We can match against some parts of browser URL instead:

const currentUrl = await browser.getCurrentUrl(); const parsedCurrentUrl = Url.parse(currentUrl); expect(parsedCurrentUrl.pathname).to.eql('/app/applink_end/some-path'); expect(parsedCurrentUrl.hash).to.eql('#/some/hash');

mshustov · 2020-06-04T13:15:50Z

test/plugin_functional/test_suites/application_links/redirect_app_links.ts

+    return browser.executeAsync<boolean>(async (cb) => {
+      const reloaded = window.__nonReloadedFlag !== true;
+      cb(reloaded);
+      return reloaded;


not needed?

Unfortunately It is, as least for TS not to complains, because our executeAsync signature is broken (and so is webdriver's but in a different way), but I got tired of adding the comment on every browser.executeAsync tbh.

kibana/test/functional/services/common/browser.ts

Lines 482 to 485 in 13138c0

public async executeAsync<R>(

fn: string | ((...args: any[]) => Promise<R>),

...args: any[]

): Promise<R> {

Agree. But TS doesn't work well with WebDriver implementation when a callback is always the last argument passed in a function.
We could change executeAsync signature to accept 1 additional argument at max:

async executeAsync<T = any>( fn: <T>(cb: (value: T) => void) => void | Promise<void>, ): Promise<T>; async executeAsync<T = any, Context extends Record<string, any> = Record<string, any>>( fn: <T>(context: Context, cb: (value: T) => void) => void | Promise<void>, context: Context, ): Promise<T>;

There are 13 tests with executeAsync in the code base, it doesn't look like a significant effort. @elastic/kibana-qa Any objections we do this change in a separate PR?

I'll defer to @dmlemeshko on this.

@elastic/kibana-qa Any objections we do this change in a separate PR?

No problem from my side.

mshustov · 2020-06-04T13:17:09Z

test/plugin_functional/test_suites/application_links/redirect_app_links.ts

+  const browser = getService('browser');
+  const testSubjects = getService('testSubjects');
+
+  const setNonReloadedFlag = async () => {


nit: here and below unnecessary async

…t-crossapp-links

joshdover · 2020-06-09T15:22:47Z

src/plugins/kibana_react/public/app_links/redirect_app_link.test.tsx

+
+  beforeEach(() => {
+    application = applicationServiceMock.createStartContract();
+    application.parseAppUrl.mockReturnValue({ app: 'targetApp' });


It doesn't appear this is used anymore? Can we remove this API now?

It isn't. I feel like I'll have to reimplement it soon enough by doing so, but you're probably right, I should remove it from this PR.

…t-crossapp-links

kibanamachine · 2020-06-11T10:48:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6e90161

History

💔 Build #53252 failed e5820f4
💚 Build #52995 succeeded 6365b2a
💔 Build #52953 failed 07cb1f5
💚 Build #52369 succeeded 7811516
💚 Build #51801 succeeded 57fabad

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

pgayvallet · 2020-06-11T13:28:39Z

@Dosant (or anyone else from @elastic/kibana-app-arch) as kibana_react is under your ownership, could we get a review on this PR?

Dosant

Gave it a try with following scenarios:

Cross link between different KP example apps without page refresh
Wrapped uiActions context menu with it and checked that link are working without page refresh

One thing to keep in mind, is that consumer have to add this wrapper for every react root. (Guess this is just like react works)

e.g.

<RedirectCrossAppLinks>
  <App>
      <a href />
     <... native js>
          ReactDOM.render(<RedirectCrossAppLinks><a href /></RedirectCrossAppLinks>)
     </ ... native js>     
  </App>
</RedirectCrossAppLinks>

A bit sad that had to explicitly pass applicationStart to use it and in that nested case above ⬆️ this is a bit of a burden

pgayvallet · 2020-06-12T11:00:54Z

One thing to keep in mind, is that consumer have to add this wrapper for every react root. (Guess this is just like react works)

Yea, unfortunately this is how react works. Roots are strictly isolated in react world. There is no alternative that wouldn't have the same limitation as my previous PR using a global handler.

A bit sad that had to explicitly pass applicationStart

Other alternative would have been to use a context, but you would have to pass the application service to the context the same way, so it doesnt change much and just add a layer of complexity imho (and that wouldnt work with 'nested' react roots either)

* implements RedirectCrossAppLinks component * update doc * review comments * use `RedirectCrossAppLinks` in SOM SO table page * update snapshots due to merge * do not filter current app * rename component * fix snapshots * add FTR tests * review comments * remove the `parseAppUrl` unused core API * fix snapshots * fix test plugin ts version * add newline

jasonrhodes · 2020-07-20T13:30:57Z

Just noting we should make sure to add these types of updates to existing docs when possible. Here's a ticket to address: #72432

implements RedirectCrossAppLinks component

f18c09f

pgayvallet added this to Pending Review in kibana-core [DEPRECATED] via automation May 28, 2020

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0 v8.0.0 labels May 28, 2020

update doc

d6337a6

pgayvallet commented May 28, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review May 28, 2020 15:16

pgayvallet requested review from a team as code owners May 28, 2020 15:16

pgayvallet added the release_note:skip Skip the PR/issue when compiling release notes label May 28, 2020

pgayvallet mentioned this pull request May 28, 2020

Improve cross-app linking in Platform #58751

Closed

Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…

4c6daa7

…t-crossapp-links

joshdover reviewed May 28, 2020

View reviewed changes

Dosant reviewed May 29, 2020

View reviewed changes

src/plugins/kibana_react/public/app_links/click_handler.ts Outdated Show resolved Hide resolved

pgayvallet added 2 commits June 2, 2020 08:36

review comments

dfd4a2c

Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…

59efaa6

…t-crossapp-links

use RedirectCrossAppLinks in SOM SO table page

208d367

update snapshots due to merge

9e4a7e1

Dosant mentioned this pull request Jun 2, 2020

Kibana developer examples landing page #67049

Merged

pgayvallet added 4 commits June 3, 2020 08:34

Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…

e904c9f

…t-crossapp-links

do not filter current app

1797e65

rename component

e0d5b4b

fix snapshots

d88d4d6

pgayvallet added 2 commits June 4, 2020 08:20

Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…

6def01a

…t-crossapp-links

add FTR tests

61f59f1

flash1293 mentioned this pull request Jun 4, 2020

Prevent page load in markdown links #68236

Closed

mshustov reviewed Jun 4, 2020

View reviewed changes

review comments

57fabad

flash1293 mentioned this pull request Jun 8, 2020

Prevent page load in field formatter links #68493

Closed

Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…

7811516

…t-crossapp-links

mshustov approved these changes Jun 8, 2020

View reviewed changes

joshdover reviewed Jun 9, 2020

View reviewed changes

pgayvallet added 3 commits June 10, 2020 08:09

Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…

a8cae8e

…t-crossapp-links

remove the parseAppUrl unused core API

07cb1f5

fix snapshots

6365b2a

joshdover approved these changes Jun 10, 2020

View reviewed changes

pgayvallet added 3 commits June 11, 2020 10:34

Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…

e5820f4

…t-crossapp-links

fix test plugin ts version

6f0a66a

add newline

6e90161

Dosant approved these changes Jun 12, 2020

View reviewed changes

pgayvallet merged commit b7057b7 into elastic:master Jun 12, 2020

kibana-core [DEPRECATED] automation moved this from Pending Review to Done (7.9) Jun 12, 2020

pgayvallet mentioned this pull request Jun 12, 2020

[7.x] add RedirectAppLinks component in kibana_react (#67595) #68996

Merged

mshustov mentioned this pull request Jun 12, 2020

Type safe browser.executeAsync #69018

Merged

smith mentioned this pull request Jun 24, 2020

Add RedirectAppLinks component to APM #69817

Closed

jasonrhodes mentioned this pull request Jul 20, 2020

Update documentation for kibana_react #72432

Closed


		import { getClosestLink } from './utils';

		const createBranch = (...tags: string[]): HTMLElement[] => {

	const link = getClosestLink(target, container);
	const link = target.closest('.containerClass a');

	public async executeAsync<R>(
	fn: string \| ((...args: any[]) => Promise<R>),
	...args: any[]
	): Promise<R> {

add RedirectAppLinks component in kibana_react #67595

add RedirectAppLinks component in kibana_react #67595

Conversation

pgayvallet commented May 28, 2020 • edited Loading

Summary

Checklist

elasticmachine commented May 28, 2020

pgayvallet May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdover left a 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

pgayvallet Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

pgayvallet Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Jun 2, 2020

Dosant commented Jun 2, 2020 • edited Loading

pgayvallet commented Jun 2, 2020

pgayvallet commented Jun 3, 2020 • edited Loading

pgayvallet commented Jun 4, 2020

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

pgayvallet Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrykkopycinski Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

mshustov Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Jun 4, 2020 • edited Loading

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

kibanamachine commented Jun 11, 2020

💚 Build Succeeded

History

pgayvallet commented Jun 11, 2020

Dosant left a comment

Choose a reason for hiding this comment

pgayvallet commented Jun 12, 2020

jasonrhodes commented Jul 20, 2020

pgayvallet commented May 28, 2020 •

edited

Loading

pgayvallet May 28, 2020 •

edited

Loading

pgayvallet Jun 2, 2020 •

edited

Loading

pgayvallet Jun 2, 2020 •

edited

Loading

Dosant commented Jun 2, 2020 •

edited

Loading

pgayvallet commented Jun 3, 2020 •

edited

Loading

pgayvallet Jun 4, 2020 •

edited

Loading

patrykkopycinski Jun 8, 2020 •

edited

Loading

mshustov Jun 4, 2020 •

edited

Loading

mshustov Jun 4, 2020 •

edited

Loading

pgayvallet Jun 4, 2020 •

edited

Loading