Improve cross-app linking in Platform

[joshdover]

Follow up issue from discussion in #58217

Currently, the Platform only supports SPA navigation between applications via the navigateToApp API. One problem with this API is that in order for applications to support both regular clicks and "open in new tab" (either via ctrl-click or right click -> open in new tab), each app has to implement a component similar to react-router-dom's <Link /> component.

We could improve this quite a bit, and we have at least two options:

• Provide a <CrossAppLink /> component in kibana_react that can call the navigateToApp and getUrlForApp APIs to create a valid href that handles both in-app navigations and open in new tab navigations.
• Provide a global DOM event listener that detects clicks on anchor tags to hrefs that reference cross-app links and overrides their behavior to automatically do SPA navigation.
  • There may be some issues with detecting intra-Kibana links vs. links to external services. Some tracking pixels installed by 3rd party plugins may be affected as well. @lizozom could you provide more details or an example of this?

[elasticmachine]

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

[pgayvallet]

Provide a global DOM event listener that detects clicks on anchor tags to hrefs that reference cross-app links and overrides their behavior to automatically do SPA navigation

I'm going to look into it.

Just to be sure we are on the same page. We would add a global listener, but only change the behavior of links having a relative url matching the path of a registered app, correct?

I.E

'/app/dashboard/some-path' // -> match
'/login' // -> match
'http://foo.bar' // -> no match
'http://current-kibana-instance/basepath/app/dashboard' -> no match

@Dosant @joshdover

[Dosant]

@pgayvallet IMO it also makes sense to check absolute links and makes sure they have same behaviour as relative? Why not?
I assume if someone, for some reason, wants to do a full page reload, they will be able to force with JavaScript.

[pgayvallet]

IMO it also makes sense to check absolute links and makes sure they have same behaviour as relative? Why not?

Basically just because it adds some complexity to the parsing process. But I guess with the same relativeToAbsolute trick we are doing elsewhere, it could easily be added

kibana/src/core/public/application/application_service.tsx

Lines 380 to 385 in aa8cb62

	function relativeToAbsolute(url: string) {
	// convert all link urls to absolute urls
	const a = document.createElement('a');
	a.setAttribute('href', url);
	return a.href;
	}

[pgayvallet]

WIP implementation works with relative urls (either with or without base-path) and absolute urls:

interface ParsedApp {
  app: string;
  path?: string;
}

/**
 * Parse given url and return the associated app id and path if an app matches.
 * Input url can either be:
 * - an path relative to the basePath, ie `/app/my-app/some-path`
 * - an path containing the basePath, ie `/base-path/app/my-app/some-path`
 * - an absolute url matching the `origin` of kibana instance (as seen by the browser),
 *   i.e `https://kibana:8080/base-path/app/my-app/some-path`
 */
export const parseAppUrl = (
  url: string,
  basePath: IBasePath,
  apps: Map<string, App<any> | LegacyApp>
): ParsedApp | undefined => { [...] }

Does that seems to be answering all use cases we want to cover?

[joshdover]

 * - an path relative to the basePath, ie `/app/my-app/some-path`
 * - an path containing the basePath, ie `/base-path/app/my-app/some-path`

What happens if the basePath is set to app? I think we should only handle the second case, a path containing the basePath since I believe that is the only case that will work with cmd-click or right-click -> open in new tab anyways?

[pgayvallet]

What happens if the basePath is set to app

/app/app/appId will work properly, as we are triming the basePath before checking for app paths. /app/appId would fail though, as it would be transformed to /appId which would not match any app route. Good catch.

I think we should only handle the second case, a path containing the basePath since I believe that is the only case that will work with cmd-click or right-click -> open in new tab anyways?

I was sure the <base> meta tag was used, but TIL it's not present even when we are using a basePath, meaning that you are right, /app/foo would not work in a normal link, only /base-path/app/foo would.

Will remove the /app/my-app/some-path option then, thanks.

[pgayvallet]

So, I created #65164 to test the global listener approach. Manual tests are working fine, however this is breaking almost all the CI FTR tests.

I need to look a little closer to the failed tests, but I'm starting to wonder if this really should be enabled 'by default' on all links.

[cjcenizal]

I'm starting to wonder if this really should be enabled 'by default' on all links.

Have we considered the other option Josh proposed? Instead of a component, I'd suggest a service called getAppLink that accepts a config for the app you want to navigate to, and returns properties that you can apply to an HTML link, spread into a React link component, or otherwise consume in JS.

getAppLink('discover', { path: '/some-path' });
/*
Returns:
{
  href: '/discover/some-path',
  onClick: () => {}, // a callback
}
*/

// Apply to a link to support SPA-navigation or opening the link in a new tab.
<EuiLink {...getAppLink('discover', { path: '/some-path' })}>Click me</EuiLink>

I like how the consumer is explicitly choosing to use this service. Default behavior of events isn't being prevented by some distant code, so it seems like it would be easier for consumers to debug.

[pgayvallet]

Have we considered the other option Josh proposed? Instead of a component, I'd suggest a service called getAppLink that accepts a config for the app you want to navigate to

We discussed it with the team and we had a preference for the other option mostly because it avoids developer errors, by ensuring every cross-app link is automatically wired to SPA navigation. It also seemed that would be a better developer experience.

Also as application.navigateToApp is a thing, the getAppLink approach has (imho) little value, as the difference between

<EuiLink {...application.getAppLinkProps('discover', { path: '/some-path' })}>Click me</EuiLink>

and

<EuiLink onClick={e => e.preventDefault(); application.navigateToApp('discover', { path: '/some-path' })}>Click me</EuiLink>

seems minimal (even if I agree that the first is more graceful).

The {...application.getAppLinkProps('discover', { path: '/some-path' })} also got the limitation that you would not be able to add additional onClick behavior to your link, as the spreaded attributes will already contains an onClick property that you would not be able to enhance (not easily without wrapping it at least).

#65164 is now passing CI, so we might be good with this global handler.

I like how the consumer is explicitly choosing to use this service. Default behavior of events isn't being prevented by some distant code

I agree that auto-magical things can sometime be dangerous, which is why I added a way to explicitly disable the 'auto navigation' handler on some links by the addition of a specific css class. I still think that the most common use case (like more than 98%) of cross-app links will be 'plain' links with only an href and no additional onclick behavior, which is why this active/explicit disabling seems sufficient. But I'm open to discussion if anyone sees additional arguments against the global handler.

[pgayvallet]

I closed #65164 due to too many complications related to handler execution order.

Options still on the table:

Provide a component in kibana_react that can call the navigateToApp and getUrlForApp

2. (alternative to 1.)

We may even able to provide a similar experience via a top-level component that listens for these events on all child components. I'd prefer this over a component that has to be used by each individual tag.

I'd suggest a service called getAppLink that accepts a config for the app you want to navigate to, and returns properties that you can apply to an HTML link,

[joshdover]

Update from a conversation outside Github:

We're going to explore next creating a wrapper React component that will essentially behave the same as the global event handler approach we explored before, but be contained to a React UI tree.

Example usage:

<RedirectCrossAppLinks navigateToUrl={application.navigateToUrl}>
 <MyCustomComponentWithLinkToAnotherApp />
</RedirectCrossAppLinks>

This RedirectCrossAppLinks component would handle event delegate via a simple loop. Quick and dirty example:

const RedirectCrossAppLinks = ({ navigateToUrl, children }) => {
  const onClick = (e) => {
    let anchor = e.target;
    while (anchor.tagName !== "A") {
      if (anchor.parentElement === null || anchor === e.currentTarget) {
        console.log(`is not inside an <a> tag`);
        return;
      }

      anchor = anchor.parentElement;
    }
    
    e.stopPropagation();
	navigateToUrl(anchor.attributes.href);
  };

  return (
    <div onClick={onClick}>{children}</div>
  );
};

Reason we prefer this over the CJ's proposal is mostly that it allows us to avoid having to couple leaf components deep in a UI tree to a Core API. This should make these more flexible.

This pattern does deviate from "React best-practices" since it relies on event delegation. However we think this may be acceptable since this behavior can be viewed as an "enhancement" to the default behavior and it does not introduce a new data flow that needs to be tested by consumers.

[jasonrhodes]

Just throwing my $0.02 in here: I'd much prefer an explicit component to use that says "I want my link to behave like a Router-based link" rather than some kind of magic operating on my links in secret behind the scenes.

I understand how it avoids accidental developer error, and that makes sense. But this kind of magic scares me :angular: 😨 I guess I'm exactly in line with CJ on this part:

I like how the consumer is explicitly choosing to use this service. Default behavior of events isn't being prevented by some distant code

[cjcenizal]

@pgayvallet In a point you made, you proposed that this was a viable alternative:

<EuiLink onClick={e => e.preventDefault(); application.navigateToApp('discover', { path: '/some-path' })}>Click me</EuiLink>

I just want to point out that without an href prop users won't be able to open this link in a new tab.

The {...application.getAppLinkProps('discover', { path: '/some-path' })} also got the limitation that you would not be able to add additional onClick behavior to your link, as the spreaded attributes will already contains an onClick property that you would not be able to enhance (not easily without wrapping it at least).

I think you raise a good point here. You'd have to wrap the returned onClick. But looking at the result, isn't it about the same thing you'd have to do if you were to use navigateToApp instead?

const { href, onClick } = getAppLink('discover', { path: '/some-path' });
const wrappedOnClick = (event) => {
  trackUiMetric('click', 'clickMeLink');
  onClick(event); // TS can alert consumers who forget the event argument
};

<EuiLink href={href} onClick={wrappedOnClick}>Click me</EuiLink>

Anyway, this is moot if we're using RedirectCrossAppLinks. I just wanted to flesh out my thoughts on this further. :)

[mattkime]

I was just dealing with this and with throw in my 2¢ -

I think we need something that would be composable with Eui components. WIP code - https://github.com/elastic/kibana/pull/66781/files#diff-76785e16a195e42ac817f1a982269d11R36

I think a function that returns OnClick and href values is a good solution. Better integration of additional OnClick functionality is possible but we'd need to define what specifically that would be. Someone could always write a function that calls reactRouterNavigate (my fn that returns onClick and href) in their code and return something else.

---

Rant warning

I see two problems with application.navigateToApp and they're both strings. The app id and the path. If you're going to link to an app, you should be required to call a method off that app's start lifecycle contract. This eliminates runtime errors and takes full advantage of typescript.

I see the same problem with the path and it needs a similar solution. Heck, apps should have this internally - and they often do through string composition. Which is okay, but I see something like -

indexPatternManagementStart.navigateTo.home();
indexPatternManagementStart.navigateTo.create();
indexPatternManagementStart.navigateTo.create({defaultValues: {pattern: '.kibana'}});
indexPatternManagementStart.navigateTo.pattern(id);
indexPatternManagementStart.navigateTo.patternField(id, fieldName);

Perhaps these methods would be backed by navigateToApp. I guess internal usage would likely call the local ScopedHistory.

URLs would become an expression of app state and api calls rather than a primary interface as it is now. URLs should be seen as the domain of external entry into Kibana.

If these ideas are worthwhile, then we don't want Provide a global DOM event listener that detects clicks on anchor tags to truly be global, but as wrapper around apps and portions of apps that aren't using the best available APIs.

It should be noted that there's an interesting similarity between internal and external url expressions and routing. Worth exploring. What if it could all be defined in a single place?

If we need a test case, I can recommend index pattern management.

Anyway, I'm not sure how possible any of this is but I know it would avoid a lot of the problems I've seen.

[mattkime]

I think I've strayed from the topic at hand. I think this problem set requires a good and agreed upon problem definition. The initial problem statement in the issues description is good but we might want to revisit it if simply to say "we're not going to solve these other specific problems" and create new tickets.

[cjcenizal]

@mattkime You might be interested in #66682, which suggests the kind of contract you describe.

[sorenlouv]

Slightly tangential (sorry!):

@mattkime @cjcenizal Completely agree. Links as pure strings are inherently unsafe.
On APM we have plans to solve this by inferring links from our client side routes (based on react-router). So if a route is modified, or query params changed, the existing links will fail at compile time and prevent the developer from linking to a route that doesn't exist.
Details: #51963

[jasonrhodes]

If you're going to link to an app, you should be required to call a method off that app's start lifecycle contract.

I like this line of thought a lot.

[joshdover]

Just throwing my $0.02 in here: I'd much prefer an explicit component to use that says "I want my link to behave like a Router-based link" rather than some kind of magic operating on my links in secret behind the scenes.

Are there any actual problems you anticipate from this behavior? And just to clarify, this is still opt-in behavior. You can add this wrapper component in your UI tree (or just parts of it) if you want, but it's not required. It's a convenience pattern for enhancing the default behavior.

I know we prefer "one way to do things", but I don't necessarily see a problem with having both options available. I believe they would even be interoperable, meaning that a UI component that uses the getAppLinkProps utility should be able to work without issues, even inside a UI tree using the <RedirectCrossAppLinks> wrapper component.

If you're going to link to an app, you should be required to call a method off that app's start lifecycle contract. This eliminates runtime errors and takes full advantage of typescript.

I agree with this, but also agree that it's orthogonal to this issue. I believe this is what the URL generator pattern is supposed to solve?

I don't think apps should expose a navigateTo method, but instead a buildUrl method(s). I prefer buildUrl since it's more lightweight and isn't coupled to a specific Core API. URLs are also more portable and can be used for lots of things (in-app navigation, sending a link in an HTTP response, etc.)

[jasonrhodes]

If it's opt-in, that's good. I was imagining this was going to wrap the entire Kibana app outside of our individual apps. Personally I'd still discourage folks from using this, because reading the code becomes harder (how did this EuiLink work right?? oh right there's a magical event listener at the root of our app that's handling it for us...) but as long as plugins don't have to opt-in, I can live with it. :D

[pgayvallet]

@cjcenizal

const { href, onClick } = core.application.getAppLink('discover', { path: '/some-path' });
const wrappedOnClick = (event) => {
  trackUiMetric('click', 'clickMeLink');
  onClick(event); // TS can alert consumers who forget the event argument
};

<EuiLink href={href} onClick={wrappedOnClick}>Click me</EuiLink>

The main issue with that approach is that the expected onClick signature for a react components is not the same as a vanilla dom handler. One expects a react specific React.MouseEvent, the other one a vanilla MouseEvent. These event types does not share the same signature (specifically, the target is not accessible in the same way: target vs nativeEvent.target), which would make the core getAppLink API non framework-agnostic if we were to implement the API in a react-compatible way, which is something that is, unfortunately, not acceptable.

@mattkime

If you're going to link to an app, you should be required to call a method off that app's start lifecycle contract

Issue with this option is that many apps got cyclic links (appA links to appB which also links to appA). Exposing navigateTo.XXX to plugins start contracts would create cyclic dependencies. I know you already replied to #66682, but linking it for the record.

@ all

I think this problem set requires a good and agreed upon problem definition.

I know we prefer "one way to do things", but I don't necessarily see a problem with having both options available

Overall:

• The implemented solution(s) should be something that our actual consumers are ok to (and would) use
• Exposing multiple ways to do something is not an issue per say, but if one of the option was to not be used by any plugin developers, it feel like unnecessary to implement it, which is why we should probably initially focus on the solution answering the consumers need the most.

I have no issue implementing a <CrossAppLinks> component in kibana_react as an initial solution if consumers feels like this is the best approach for them. however we also have to consider the limitation of such component:

As the component will need to call a core API (application.navigateToApp or application.navigateToUrl - #67110), the component will need access to the core service.

Which mean the actual usage would be either using a direct reference, forcing to pass down the core service alongs all the react component tree:

<CrossAppLink navigateToUrl={core.application.navigateToUrl} url="/base-path/app/dashboard/path"/>

<CrossAppLink navigateToApp{core.application.navigateToApp} app="dashboard" path="/some=path"/>

Or using a context provider (which is probably the best option, even if it also has drawbacks, such as harder testability)

<CrossAppLinkContext navigateToUrl={core.application.navigateToUrl}>
  ...
     <CrossAppLink url="/base-path/app/dashboard/path"/>
  ...
<CrossAppLinkContext />

<CrossAppLinkContext navigateToApp={core.application.navigateToApp}>
  ...
   <CrossAppLink app="dashboard" path="/some=path" />
  ...
<CrossAppLinkContext />

[mattkime]

My main concern about the 'global' solution was that it would be truly be global. if its opt in then I see no problem.

[joshdover]

<CrossAppLinkContext navigateToUrl={core.application.navigateToUrl}>
  ...
     <CrossAppLinks url="/base-path/app/dashboard/path"/>
  ...
<CrossAppLinkContext />

I think we're still not quite aligned on this API, this is how I imagined it being used:

<RedirectCrossAppLinks
  navigateToUrl={coreStart.application.navigateToUrl}>
  {/** These would automatically get handled by RedirectCrossAppLinks */}
  <a href={`/app/dashboards/${dashboardId}`}>Go to Dashboard</a>
  <EuiLink href={`/app/dashboards/${dashboardId}`}>Go to Dashboard</EuiLink>
</RedirectCrossAppLinks>

It would be up to the app to wire RedirectCrossAppLinks up to navigateToUrl but this could be done at the outermost layer, so it shouldn't necessarily need to use a context.

RedirectCrossAppLinks would need to handle onClick events on all child elements via event bubbling, but it wouldn't require a special component with a url prop like in your example: <CrossAppLinks url="/base-path/app/dashboard/path"/>.

Are there any issues with this approach I'm missing? If I'm still not clear, I can do a quick POC as well.

[pgayvallet]

I think we're still not quite aligned on this API, this is how I imagined it being used:

Sorry if I was unclear. The CrossAppLinkContext + CrossAppLinks approach I proposed was another option than the RedirectCrossAppLinks one.

If everyone is fine with the <RedirectCrossAppLinks> react component approach, I have no problem implementing it.

As I already said on slack, the only thing I don't like about this option is that doesn't quite follow the React way of doing things (global handlers and side effects are not really in React's philosophy). However this is not a technical limitation, and I have to admit this option is the easier to use for consumers, so If no one is opposed, I'm fine trying it.

[joshdover]

Sorry if I was unclear. The CrossAppLinkContext + CrossAppLinks approach I proposed was another option than the RedirectCrossAppLinks one.

Got it, after re-reading, it was clear and I just missed it 😄

[pgayvallet]

The RedirectCrossAppLinks component approach is ready for review in #67595

[pgayvallet]

#67595 has been merged, however I'll keep this issue open for discussion on the eventual additional tools we could provide as alternatives to <RedirectCrossAppLinks />

[pgayvallet]

Closing. Feel free to reopen if you think the discussion can continue.