Plugin URLs should be part of the plugin's contract

[cjcenizal]

For an example of the problem see https://github.com/elastic/kibana/pull/65796/commits/68c0eb47bd052a0ec5b03821df865f67a227792f. This PR changed the paths of several plugins, which required manually finding and updating the points of entry into these plugins from other plugins.

Changing a plugin's URL should be considered a breaking change from the perspective of dependent plugins. It's easy to miss a plugin's dependency upon another plugin's URL, but by requiring plugins to publish entry points into their apps we can force dependent plugins to identify and declare this dependency via kibana.json (and properly handle the case in which the dependency is missing).

[elasticmachine]

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

[pgayvallet]

I'm guessing that by 'plugin' you mean Application's url or route. (a plugin can have multiple applications registered)

Imho:

• application.navigateToApp(app, { path }) answer this need, by abstracting the basePath of an application.
• changing any plugin's app/route url is indeed breaking any dependent plugin that are performing navigation to the dependency plugin's routes. However ideally, these would be catched by FTR test coverage, as we should have tests asserting these scenarios.

However, the example you linked is an issue about a plugin changing some of their routes, which is different than changing the app's basePath. This is not something we can properly detect from core, as we are only aware of an application's basePath, not their route mapping.

I.E if the foo plugin decided to change their /bar route to /dolly Existing application.navigateToApp('foo', '/bar') would lands on a 404, as the /bar route was moved, however this is something we can't detect from core.

However, in these cases:

• Plugins should probably handle transition period by properly handling / redirecting from previous routes to new ones (not always possible).
• To avoid these risks, plugins could (should?) expose navigation helpers from their APIs

I.E

props.application.navigateToApp(
    'kibana#/management/elasticsearch/license_management'
);

could be changed for something like

props.managementStart.navigateToSection('elasticsearch', 'licensing_management')

to let the dependency plugin handles the action-to-navigation logic.

[cjcenizal]

However ideally, these would be catched by FTR test coverage, as we should have tests asserting these scenarios.

Tests are a great point, but FTR tests are particularly slow and difficult to develop. As I result I think people tend to avoid them if possible. Would there also be an added cost for disabling the dependency plugin as part of the test? I haven't looked into this, but I imagine it would require a Kibana server restart. If we could solve this problem more cheaply (e.g. via TS checks), I'd prefer that.

To avoid these risks, plugins could (should?) expose navigation helpers from their APIs

Right! This is exactly what I had in mind. Except in your example, I don't think the Management plugin could expose this helper, since it's not aware of License Management. License Management should expose this helper and any plugin that contains an app that wants to link to it should declare it as a dependency.

[pgayvallet]

To avoid these risks, plugins could (should?) expose navigation helpers from their APIs

License Management should expose this helper and any plugin that contains an app that wants to link to it should declare it as a dependency.

After thinking about it a little more I think this will quickly cause cyclic dependency issues:

I.E: dashboard app can link to discover, discover can link to dashboard, but including both as a dependency to the other is not possible.

Not sure what our best option is then

[cjcenizal]

Ouch, you're right. What if the platform introduced a new type of dependency, e.g. staticPlugins or nonTransitivePlugins? Would plugins be able to define a particular set of exports that they can guarantee don't depend upon other plugins? This seems like an overengineered solution if the only problem we're trying to solve is guarding against broken links, so I'd lean away from it unless we think there are other problems it would also solve.

[pgayvallet]

This seems like an overengineered solution if the only problem we're trying to solve is guarding against broken links, so I'd lean away from it unless we think there are other problems it would also solve.

Basically that yea. I wouldn't want to recode spring multi-step DI just for this specific need :D

[mattkime]

Voicing support for @cjcenizal 's thoughts. I fully agree with what he's proposing and why - it seems like the primary argument against it is that we need to find an acceptable way to implement it.

Do optional dependencies do anything to fix the cyclical dependency issue? I assume not but have to ask.

Still, there's a clear solution to the problem - rely on core or another plugin to provide the interface to register and look up app URLs. You'd need to specify the type when retrieving but that extra step would provide everything else we want.

This would help solve the problem of having internally consistent urls. Yes, we should maintain backward compatibility with urls stored externally but I think that needs to be solved in routing.

---

I should mention that the URL Generator exists - https://github.com/elastic/kibana/issues/25247 - but I'm really not crazy about registering one url at a time. Slightly different usage and api.

[pgayvallet]

Do optional dependencies do anything to fix the cyclical dependency issue

Unfortunatly it doesn't. Both kind of dependencies are used the same way when generating the dependency graph, that cannot be cyclic for multiple reasons.

[mshustov]

dashboard app can link to discover, discover can link to dashboard, but including both as a dependency to the other is not possible.

How it's solved right now? App A cannot provide a link to App B if it's unavailable for the current user, it means that App B doesn't register itself (in a registry or core API) 🤔

[pgayvallet]

How it's solved right now

I didn't have a specific example in mind for the dashboard to/from discover example. I don't even know if it's a valid one tbh, I just used it to state the potential cyclic dependency issue of navigating using the plugin's contract). However as the legacy kibana plugin was previously owning multiple apps that got separated into distinct plugins, I feel like these dual-way app linking are a real issue (Maybe @elastic/kibana-app could confirm?)

But technically, nothing blocks an app to use application.navigateTo('disabledApp') atm (or even just create a <a> link to a disabled app for instance). It would just display the 404 page.

[timroes]

Everything what CJ describes here is the way we want to go. No application should build any kind of URL in some kind of string for any other application. You should only consume URLs via APIs of that specific plugin so the ownership of creating those URLs is clearly within that plugin.

As pointed out we'll run into the cyclic dependency problems with it, since apps can pretty cross link between each other. We have that case in a couple of places in Kibana (e.g. the whole observability apps).

The above linked was afaik meant to target that problem (I haven't followed it's development closely though). If it now has become something different, or different APIs are needed to be used, we should make this clear. This is one of those topics we imho need a very clear guidance on how plugins should do that.

@stacey-gammon can hopefully clearify on the "how should this be done" part.

[stacey-gammon]

URL generators aren't meant to solve this. Pulling specific implementations off a generic registry should not be used as a way to circumvent circular dependencies - it'll still be an issue, just not as obvious.

The only way I can see to get around this is to build separate plugins. One with the application contents, one with the linkers. Then the chain is:

Dashboard App -> Discover Link Discover App -> Dashboard Link

It would encourage many small plugins though, and it's still weird because shouldn't Dashboard Link have a dependency on Dashboard App? How else could you test it? What good is a dashboard link plugin without a dashboard app plugin?

I brought my concern up with circular dependencies in https://github.com/elastic/kibana/issues/47065. The sentiment was:

Regarding circular dependencies, I think when these arise it's more indicative of a code smell than an architectural problem with the Platform. Typically, this indicates tight-coupling or a need for reorganization.

100% agree with this. If we hit a circular dependency, we should have a bias toward finding out whether it truly needs to be there in the first place,

I don't think we considered inter app linking then, and it's a great example of a circular dependency that doesn't seem to me like a code smell.

tl;dr; I don't have a recommended answer for "how this should be done" but agree it's something we need to figure out.

cc @joshdover @ppisljar @lukeelmers

[pgayvallet]

I don't think we considered inter app linking then, and it's a great example of a circular dependency that doesn't seem to me like a code smell.

+1 on that

[lukeelmers]

Yeah I don't think we were considering linking between apps as an example of circular dependencies in that original discussion -- IMHO having a bunch of separate "linking" plugins is just moving our code smell from one place to another 😄

That said, I'm not sure I can conceive of a way to link between apps without either:

1. circular dependencies
2. workarounds like the URL generators which create implicit dependencies; or
3. making our domains vastly more complex by splitting things into a bunch of nav-specific plugins which folks would need to keep track of

My gut reaction: Since you can have circular dependencies at the plugin level if they are type-only imports, I wonder if the least-worst option would be a combination of 1 and 2: You use a registry to get your URL (either URL generators or persistable states + core.application.navigateToApp). Then rely on type-only imports to retrieve the relevant state interfaces from the plugin you are navigating to.

This is at least somewhat more explicit due to the type imports, though certainly not as ideal as the original proposal of exposing these methods from each plugin.

This needs more thought though -- would be interested to hear others' ideas on this.

[stacey-gammon]

You use a registry to get your URL (either URL generators or persistable states + core.application.navigateToApp).

Still, there's a clear solution to the problem - rely on core or another plugin to provide the interface to register and look up app URLs.

I don't think this will solve the root problem, and is going against our current recommendations of not going through a generic registry when you have concrete information at compile time.

It feels like a hacky solution that could start causing flaky ci errors that are hard to debug.

For instance. Hey, this works!

class FooPlugin {
 setup(core) {
    core.registerApp(() => {
       // Post start lifecycle so this works without an explicit dependency
       const [coreStart, { urlGenerators } ] = core.getStartServices();
       renderApp(linkToDiscover: urlGenerators.get(DISCOVER).createUrl());
    })
  }
}

Hm, but this randomly breaks ci:

class FooPlugin {
 setup(core) {
    core.registerApp(() => {
       // Post start lifecycle so this works without an explicit dependency
       const [coreStart, { urlGenerators } ] = core.getStartServices();
       const linkToDiscover = urlGenerators.get(DISCOVER).createUrl();
       renderApp(linkToDiscover);
    }
  }

  start(core, { urlGenerators }) {
    // This will cause random ci failures without an explicit dependency on the discover plugin.
    const linkToDiscover = urlGenerators.get(DISCOVER).createUrl();
    return {
      DashboardEmbeddable: () => <DashboardEmbeddable linkToDiscover={linkToDiscover} />
    }
  }

What about service level dependencies?

[stacey-gammon]

We took on this issue in today's Repo Review sync.

Summary

We recommend creating separate plugins if circular dependencies is an issue due to inter app linking.

Note the platform team is doing work to support plugins nested inside folders, which will help avoid a too flat plugin folder structure going this route.

Notes

• Who is "enhancing" who? It's bidirectional.
• Identify tradeoffs, LOE.
• Would help if we had a specific list of these inter app links and how/where they are exposed.
• Do we need to solve this generically or just for "inter app linking"?
• Small plugins are not the answer for creating public APIs that only one other plugin needs. Don't share code prematurely.

Options we have:

1. Package tightly coupled things together.

   • Would mean everything is coupled: Dashboard shouldn't be packaged with Discover, etc. No go.

2. New plugin that has all the implementation code inside. This would go against our "single owner per plugin" recommendation. Not pluggable. Tightly couples all plugins. Every plugin depends on this plugin.

3. Rely on implicit dependencies. Assume all usage is post start life cycle. This goes against our current recommendations which specifically say don't do this.

4. "Service level" dependencies (e.g. more plugins). Encourages lots of small plugins.

   • New webpack build for every front end plugin so potential performance issues, but long term we want to solve this anyway. Small plugins may be less overhead as well.
   • Discoverability of ids and access path becomes a question with lots of small plugins. Could we use namespacing?
   • dashboard.navigation.createUrl
   • dashboard.app
   • dashboard.embeddable.DashboardEmbeddable

Work through how navigation should work if the app is disabled. Should users be able to create links? Server side? Dashboard app plugin could turn off dashboard navigation plugin if disabled.

5. Possible to use triggers & ui actions? Dependency is on the trigger const and ui actions.

6. Accomplish this via "bundle refs"? E.g. same plugin but do something operationally to cause the bundles to be split up? Extra public dirs in kibana.json? "entry points" in my public directories. Bundles get created from these. Separate entry point for navigation related items.

   • This adds a lot of complexity.
   • Only works for stateless things (can't rely on anything from core).

We are recommending option 4 as a work around.

Some next steps to make this experience better:

• Namespacing so plugins can be grouped, e.g. dashboard.navigation.createUrl vs dashboardNavigation.createUrl
  • Performance improvements as this will cause more, smaller plugins.

We should still come up with a recommendation for "who's enhancing who".

For example, OSS apps shouldn't be registering themselves into x-pack plugins. X-pack plugins are the enhancers, that enhance OSS plugins. Opt for one way direction when possible.

[Dosant]

FYI

We are adding a high-level guide on routing, navigation and URL: https://github.com/elastic/kibana/pull/76888 Guide covers trivial example usages of core's navigateToApp and explains concept of url generators. It also mentions: Treat URL as part of plugin contract.

In case something new and important pops up from this discussion, then docs/developer/best-practices/navigation.asciidoc should be updated.