Add KibanaURL interface to core

streamich commented 4 years ago

This is a proposal to introduce KibanaURL interface/class into share plugin.

`KibanaURL` interface

Similar to URL class in Node.js and URL Web API the KibanaURL interface would have the following fields.

interface KibanaURL {
  hash: string;
  host: string;
  hostname: string;
  href: string;
  pathname: string;
  port: number;
  protocol: string;
  search: string;
}

But it would also have Kibana specific fields, such as below.

interface KibanaURL {
  app: string;

  // Edit: added this field after @pgayvallet comments
  path: string; // Path within Kibana app /xyz/app/dashboard/<PATH>

  go(options?: { newTab?: boolean }): void;
  navigate(options?: { replace?: boolean, newTab?: boolean }): Promise<void>;
  internal: boolean;
}

.app is a string representing the Kibana app ID, if KibanaURL is internal link into Kibana. Or empty string if link is external. The app ID could be extracted manually from pathname, but one needs the knowledge of structure of Kibana URLs to do that.

.go() is a method that navigates to a different URL using page reload. Internally it would use window.location.href and window.open() depending if newTab flag is set.

.navigate() is a method that navigates within Kibana in SPA (single page app) way. Kibana uses history and react-router packages in Core for navigation between apps. navigate() method would leverage core.application.navigateToApp to execute SPA navigation between Kibana apps. If URL is external it wold fall back onto .go() method. It is called "navigate" analogous to how it is done in react-router. replace option specifies whether to push a new record into navigation history or replace the current one. Server-side implementation would not have .navigate() method.

.internal boolean indicating whether the URL is internal within Kibana SPA.

Changes to `share` plugin contract

URL generator's createUrl method will return KibanaURL instead of string.
Alternatively, createUrl could still return a string but the generator could have another method createUrlObject (or createKibanaURL) which would return KibanaURL, as suggested here.
The share plugin will expose KibanaURL constructor, e.g new plugins.share.KibanaURL({ ... }), or new plugins.share.URL({ ... }), or plugins.share.createURL({ ... }).

Use cases

Better DX

One can encapsulate all information about internal Kibana URL into a single object. KibanaURL can easily create a full KibanaURL providing only partial information, defaults are used for the skipped fields:

const url = new plugins.share.KibanaURL({
  app: 'kibana',
  pathname: '...',
});

Developer has access to all fields, even the ones that were skipped when constructing KibanaURL.

url.host
url.port
url.pathname
url.hash
// ...

And developer has access to Kibana-specific functionality:

url.app
url.navigate()
// ...

URL generators

Currently to navigate between Kibana apps developer needs to have a 2-tuple of app ID (appID) and path within that app (appPath), so they can execute:

core.application.navigateToApp(appID, appPath)

but URL generators return a string, thus developer needs to manually extract app ID and app path from that string, which requires knowledge about Kibana URL structure.

Embeddable output

In embeddable output we have .editUrl field which is a string. Instead that could be KibanaURL.

If we don't make it KibanaURL the alternative is to add 2 new fields to embeddable output—editApp and editPath.

cc @elastic/kibana-app-arch @stacey-gammon @flash1293

flash1293 commented 4 years ago

I like this idea because it makes more explicit what an URL means - passing strings which contain URLs (as we are doing it now) is inherently dangerous because we can't type check whether they are used correctly.

This api makes it super convenient to act on the URL (because the methods are right there), but this also means that it's non-trivial to serialize and deserialize KibanaURL instances. If we use this interface with methods on top (instead of a plain JSON friendly structure plus some static helper methods), then there should be serialize/deserialize functions provided by the share plugin as well to work with them.

streamich commented 4 years ago

... there should be serialize/deserialize functions provided ...

KibanaURL could have .toString() and .toJSON() methods same as in Node.js URL and Web API URL.

Serialization could probably be achieved by formatting to string and back.

const url1 = new plugins.share.KibanaURL({app: 'dashboard', hash: '...'});

const serialized = url1.toString() // or String(url1)
const url2 = new plugins.share.KibanaURL(serialized);

String(url1) === String(url2);

stacey-gammon commented 4 years ago

I like this.

My one thought is how to handle server vs client side usage. navigate isn't useful on server, but there will be url handling code on server side (chat ops, etc).

Wonder if maybe we should have a common type, then expand that on the client side.

Somewhat similar discussion about URLs in Global Search RFC - https://github.com/elastic/kibana/pull/64284

Dosant commented 4 years ago

Does it make sense for KibanaUrl to become part of platform? Since navigation and url format seems like something fundamental?

cc @elastic/kibana-platform

Dosant commented 4 years ago

In addition to appId should we consider adding spaceId? If we add spaceId can this interfaces still be OSS?

Dosant commented 4 years ago

Kibana apps also have common hash format, which is normally an url fragment that we can parse: so it could look like:

interface KibanaURL {
  hash: string | { 
    pathname: string;
    search: string;
  };
  host: string;
  hostname: string;
  href: string;
  pathname: string;
  port: number;
  protocol: string;
  search: string;
}

Would it also make sense to have query along with search which would be parsed version of search string?

streamich commented 4 years ago

Would it also make sense to have query along with search which would be parsed version of search string?

interface KibanaURL {
  hash: string | { 
    pathname: string;
    search: {
      [key: string]: string;
    };
  };
  host: string;
  hostname: string;
  href: string;
  pathname: string;
  port: number;
  protocol: string;
  search: {
    [key: string]: string;
  };
}

Makes sense to me.

Although, hash does not have to be necessarily "Kibana-hash" it could have completely different structure. Maybe there could be kibanaHash field.

interface KibanaURL {
  hash: string;
  kibanaHash?: { 
    pathname: string;
    search: {
      [key: string]: string;
    };
  };
  host: string;
  hostname: string;
  href: string;
  pathname: string;
  port: number;
  protocol: string;
  search: {
    [key: string]: string;
  };
}

But if we put KibanaURL into core, then it probably does not make sense to have kibanaHash field, as that is specific to KibanaApp team.

lukeelmers commented 4 years ago

Overall, I'm interested to hear the perspective of @elastic/kibana-platform on this, especially WRT whether we would ever envision a concept like this living in core.

My one thought is how to handle server vs client side usage.

This is my first thought too -- from my understanding, one of the motivations for making the url generators return a string originally was to have something that could eventually be used on the server.

IMO if we aim to introduce some sort of global concept of a Kibana URL, we should consider something that's basic enough to use on the server too.

Currently to navigate between Kibana apps developer needs to have a 2-tuple of app ID (appID) and path within that app (appPath), so they can execute:

core.application.navigateToApp(appID, appPath)

but URL generators return a string, thus developer needs to manually extract app ID and app path from that string, which requires knowledge about Kibana URL structure.

This is somewhat orthogonal to the discussion, but I'm interested to hear more about this use case. If navigateToApp works for your purposes, then why do you need URL generators? Presumably to serialize some state into the URL?

I don't know the full background on the drilldowns PR you linked to, but I think this problem might be solved with the proposed persistable state registry -- which could be used to get some state for passing to navigateToApp:

navigateToApp(appId: string, options?: { path?: string; state?: any }): Promise<void>;

(And downstream apps like Dashboard would of course need to be looking for any state that's passed in via navigateToApp)

streamich commented 4 years ago

but I'm interested to hear more about this use case. If navigateToApp works for your purposes, then why do you need URL generators? Presumably to serialize some state into the URL?

Yes, the URL generator is used in dashboard drilldown to get the URL of target dashboard

Pseudocode:

const path = urlGenerator(DASHBOARD_APP_URL_GENERATOR).createUrl({});
const [appId, hash] = url.split('#');
navigateToApp(appId, {hash});

pgayvallet commented 4 years ago

Overall, I'm interested to hear the perspective of @elastic/kibana-platform on this, especially WRT whether we would ever envision a concept like this living in core

This proposal looks like a great idea. Kibana enhanced URL is a recurrent need, and having a proper solution to handles that would be a nice addition.

Imho it would make sense to have this interface in core, as this is a low-level component/api that should be used instead of strings and node's URL whenever it's possible. TBH core itself could leverage this, which would also not be possible if the API is exposed from elsewhere.

IMO if we aim to introduce some sort of global concept of a Kibana URL, we should consider something that's basic enough to use on the server too.

++ On that. AFAIK, we can have slightly different APIs on the client and server side. All the fields would be present on both interfaces, but the client-specific APIs such as go and navigate would only present on the client side.

Some remarks:

1.

interface KibanaURL {
  app: string;
  ...
}

const url = new plugins.share.KibanaURL({
  app: 'kibana',
  pathname: '...',
});

I think we should really dissociate pathname, in the URL meaning, which is the absolute path of the URL, and what we call appPath, which is the part of the pathname that goes after the app route base.

I.E

http://kibana:42/base-path/app/my-app/some/path;

pathname is /base-path/app/my-app/some/path; appPath is /some/path

Main upsides I see:

Preserve compatiblity with the URL interface (having KibanaURL being a extension of node's URL - as much as possible - would imho be really great)
Allow to create KibanaURL from either URL properties or our newly introduced ones.
Allow to access/alter this appPath property easily if needed

2.

.app is a string representing the Kibana app ID, if KibanaURL is internal link into Kibana. Or empty string if link is external. The app ID could be extracted manually from pathname, but one needs the knowledge of structure of Kibana URLs to do that.

This is where it becomes tricky for several reasons:

On the server side, we need the basePath to do that, meaning that we need the performing KibanaRequest

If we want to parse the absolute URL on the server-side to deduce the app field, we will need an actual KibanaRequest instance associated with the url to be able to retrieve the basePath that was used in the url.

I.E, with spaces plugin enabled, If we get an url such as

http://kibana:42/base-path/space/app/my-app/app-path;

We would need the performing request to retrieve the information that the basePath is /base-path/space when parsing the absolute url to deduce the app name.

Even when removing the basePath, retrieving the appName from the /app/my-app/app-path is non trivial

As applications can define a custom path with appRoute, deducing an app ID from the pathname requires the list of registered application to check every registered apps for custom routes.

On the client, there is currently no API in core for that, but exposing a subset of the routes properties from the application service to answer this need wouldn't be an issue.

On the server-side however, we got no informations about the registered applications, as registration is performed only on the client side, meaning that we currently have no way to deduce the appId from the path.

This point is very similar to the discussion here (and some other conversations on the RFC): https://github.com/elastic/kibana/pull/64284#discussion_r414903332

In summary, there is currently no way to parse an appId from an absolute or relative url from the server-side atm, which mean that if we want KibanaURL to be present on the server-side, this information would need to be manually provided when creating instances.

3.

const url = new plugins.share.KibanaURL({
  app: 'kibana',
  pathname: '...',
});

Developer has access to all fields, even the ones that were skipped when constructing KibanaURL.

Only a technical detail, but I think generating/computing/parsing the values of the missing properties will requires some core APIs access, which make me think the direct constructor approach is not viable. contract.createKibanaUrl() seems easier to implement.

4.

let a = new URL('http://foo/foo/bar')
>> URL {href: "http://foo/foo/bar", origin: "http://foo", protocol: "http:", username: "", password: "", …}
a.protocol = "https:"
a
>> URL {href: "https://foo/foo/bar", origin: "https://foo", protocol: "https:", username: "", password: "", …}

Web URL supports altering the URL properties after creating, and automatically updates the other properties (in the example, changing the protocol also updates the href and origin)

Do we want to supports this?

streamich commented 4 years ago

@pgayvallet

... currently have no way to deduce the appId from the path.

Maybe you could use a heuristic that appId follows /app, like

/app/<APP_ID>/app-path

Web URL supports altering the URL properties after creating, and automatically updates the other properties (in the example, changing the protocol also updates the href and origin)

Do we want to supports this?

My first instinct would be on contrary to make it immutable.

interface KibanaURL {
  readonly hash;
  readonly host;
  // etc.
}

Do we ever want to edit the URL inline?

If we want to "mutate" it, maybe we can consider that KibanaURL is immutable, but it could have a way to create a new KibanaURL using the information of the existing one.

const url1 = new core.URL({
  app: 'kibana',
  path: '/dashboard/1',
});

String(url1) // http://localhost:5603/xvo/app/kibana/dashboard/1

const url2 = url1.extend({
  path: `${url1.path}/visualization/5`
});

String(url2) // http://localhost:5603/xvo/app/kibana/dashboard/1/visualization/5

pgayvallet commented 4 years ago

Maybe you could use a heuristic that appId follows /app, like /app//app-path

Apps can define custom appRoute path than can be outside of the /app prefix:

https://github.com/elastic/kibana/blob/48a33abdeed147932401cc3a24c36669189f67f3/src/core/public/application/types.ts#L220-L225

So unfortunately we can't without the whole list of registered apps and their associated routes.

I.E

application.register({
   id: 'my-app',
   appRoute: '/foo/bar'
   ...
})

const url1 = new core.URL({
  app: 'my-app',
  path: '/dashboard/1',
});

// would need to return http://localhost:5603/xvo/foo/bar/dashboard/1
// and not http://localhost:5603/xvo/app/my-app/dashboard/1
String(url1)

My first instinct would be on contrary to make it immutable. Do we ever want to edit the URL inline?

I'm fine with immutability and extend approach instead of mutable attributes. The technical challenges regarding dependencies between fields remains the same though.

streamich commented 4 years ago

The technical challenges regarding dependencies between fields remains the same though.

Maybe the logic of constructing the URL could be in one place—constructor. And only there. The .extend() method would re-use the logic of constructor.

interface KibanaURLJSON {
    readonly hash;
    readonly host;
    // etc.
}

type KibanaURLParams = Partial<Pick<KibanaURLJSON, ...>>;

class KibanaURL implements KibanaURLJSON {
  private readonly json: KibanaURLJSON = {};
  public toJSON() { return this.json; }
  public get hash() { return this.json.hash; }
  public get host() { return this.json.host; }
  // etc.

  constructor(params: KibanaURLParams) {

    // Some processing. All the logic goes here.

    this.json = { ... };
  }

  extend(params: KibanaURLParams) {
    return new KibanaURL({
      ...this.json,
      ...params,
      ...(params.href ? parse(params.href) : {})
    });
  }
}

Idea being that when constructor is called there are also technical challenges regarding field dependencies.

Actually, it is confusing. Maybe .extend() should not allow href and other combined fields.

  extend(params: Omit<KibanaURLParams, 'href'>) {
    // ...
  }

🤷‍♂️

kobelb commented 4 years ago

I agree with @pgayvallet that this is likely something that should be part of core, and not scoped to just the share plugin.

pgayvallet commented 4 years ago

Still very in favor of implementing this as a part of core, however we still got some unresolved questions:

Majors

Does this needs to be on the server-side too, or is client-side good enough for now

See https://github.com/elastic/kibana/issues/64497#issuecomment-621089241: We currently have no way to create a proper KibanaUrl using the appId from the server, meaning that currently, KibanaURL would need the actual path to be created from the server-side. Adding to that the fact that the sugar methods such as navigate are not usable on the server, I think client-side is the priority here, and implementing it only on this side is probably acceptable?

Constructor parameters

Should we really allow to create a KibanaURL instance from all its properties, or we we want to have two helpers methods, one to create with 'raw' url properties + one to create using 'kibana' url properties

I'm just a little afraid of having to handle error cases when handling all parameters.

A quick example would be:

core.url.create({
   application: 'dashboard',
   path: '/foo/bar',
})

application and path are mutually exclusive here, as both are tightly coupled (setting one would alter the other). What should we do in that case? throw an error? Assume that one got higher priority than the other?.

I feel like having 'helpers' instead would help address the issue:

// would 'fallback' to URL to create the url string, then use the same mechanism we use in
// `application.navigateToUrl` to determine if it's an internal url or an external one, and parse the appId/appPath in
// case of internal url.
core.url.create(url: string);
// close to `application.navigateTo`parameters, with the addition of `basePath` to allow creating url to other spaces for example. 
// `basePath` would 'default' to the current basePath
core.url.create({
   app: string,
   path: string,
   basePath?: string
});
// pretty much the same logic as the `core.url.create(url: string);` version.
core.url.create({
  hash?: string;
  host?: string;
  hostname?: string;
  href?: string;
  pathname?: string;
  port?: number;
  protocol?: string;
  search?: string;})

Note: we can have distinct names for these methods, or just use overrides as in the example, not sure which is the best.

Important: the potential extend API is directly linked/related to this point.

Minors

KibanaURL instance construction

As already stated, the KibanaURL concrete instance would require access some core API, which is why it would be very easier for us to expose factory method instead of exposing a concrete class constructor

// KibanaURL is a class, harder to implement
const url = new core.URL(...);
// KibanaURL is just an interface, way easier to implement
const url = core.url.create(...);

Is that alright with everyone?

go + navigate overlaps

go(options?: { newTab?: boolean }): void;
navigate(options?: { replace?: boolean, newTab?: boolean }): Promise<void>;

.go() is a method that navigates to a different URL using page reload. Internally it would use window.location.href and window.open() depending if newTab flag is set. .navigate() is a method that navigates within Kibana in SPA (single page app) way. Kibana uses history and react-router packages in Core for navigation between apps. navigate() method would leverage core.application.navigateToApp to execute SPA navigation between Kibana apps. If URL is external it wold fall back onto .go() method.

Unsure we need the two tbh, I feel like navigate should be enought as you included the newTab parameter. As navigate already fallbacks to using window.location if the url is not internal, go would only be used is when we want to navigate to an internal url without using SPA navigation, which doesn't really seems like a real use case?

KibanaURL.toString()

What is this supposed to do? Return the string representation of the url (url.href), right?

const url = core.url.create({ app: 'dashboard', path: '/foo?bar=hello'})
url.toString(); // `https://kibana-instance/s/my-space/app/dashboard/foo?bar=hello`
url.toString() === url.href // true

streamich commented 4 years ago

My two cents:

Does this needs to be on the server-side too, or is client-side good enough for now

All uses cases I know for AppArch and KibanaApp are on client-side. So, just client-side would be good, AFAIK.

Should we really allow to create a KibanaURL instance from all its properties...

I would suggest to go for whatever is easiest, i.e. some properties.

... it would be very easier for us to expose factory method instead of exposing a concrete class constructor

:+1:

go + navigate overlaps

Maybe one is enough. Only thing if one wanted the navigate within the Kibana but with reload, they would need to do it themselves.

KibanaURL.toString()

Yes, serialize KibanaURL to string URL.

streamich commented 4 years ago

Not sure how difficult and if possible, but we ran into a use case where an app (e.g. "feature") can be disabled in Space configuration or Role. Maybe in those cases the KibanaURL could know that the destination is not enabled:

const url = core.url.create({ app: 'discover', path: '...' });
url.isReachable(); // true / false

streamich commented 4 years ago

👆 it could use

core.application.capabilities?.<app>.show

elastic / kibana