-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add KibanaURL interface to core #64497
Comments
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 |
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); |
I like this. My one thought is how to handle server vs client side usage. Wonder if maybe we should have a Somewhat similar discussion about URLs in Global Search RFC - #64284 |
Does it make sense for cc @elastic/kibana-platform |
In addition to |
Kibana apps also have common hash format, which is normally an url fragment that we can parse:
Would it also make sense to have |
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, 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 |
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 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.
This is somewhat orthogonal to the discussion, but I'm interested to hear more about this use case. If 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(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 |
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}); |
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
++ On that. AFAIK, we can have slightly different APIs on the client and server side. Some remarks: interface KibanaURL {
app: string;
...
} const url = new plugins.share.KibanaURL({
app: 'kibana',
pathname: '...',
}); I think we should really dissociate I.E
Main upsides I see:
This is where it becomes tricky for several reasons:
If we want to parse the absolute URL on the server-side to deduce the I.E, with
We would need the performing request to retrieve the information that the basePath is
As applications can define a custom path with On the client, there is currently no API in core for that, but exposing a subset of the routes properties from the 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): #64284 (comment) 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 const url = new plugins.share.KibanaURL({
app: 'kibana',
pathname: '...',
});
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. 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 Do we want to supports this? |
Maybe you could use a heuristic that
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 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 |
Apps can define custom kibana/src/core/public/application/types.ts Lines 220 to 225 in 48a33ab
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)
I'm fine with immutability and |
Maybe the logic of constructing the URL could be in one place—constructor. And only there. The 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(params: Omit<KibanaURLParams, 'href'>) {
// ...
} 🤷♂️ |
I agree with @pgayvallet that this is likely something that should be part of core, and not scoped to just the share plugin. |
Still very in favor of implementing this as a part of core, however we still got some unresolved questions: Majors
See #64497 (comment): We currently have no way to create a proper
Should we really allow to create a 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',
})
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 Minors
As already stated, the
Is that alright with everyone?
go(options?: { newTab?: boolean }): void;
navigate(options?: { replace?: boolean, newTab?: boolean }): Promise<void>;
Unsure we need the two tbh, I feel like
What is this supposed to do? Return the string representation of the url ( 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 |
My two cents:
All uses cases I know for AppArch and KibanaApp are on client-side. So, just client-side would be good, AFAIK.
I would suggest to go for whatever is easiest, i.e. some properties.
👍
Maybe one is enough. Only thing if one wanted the navigate within the Kibana but with reload, they would need to do it themselves.
Yes, serialize |
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 const url = core.url.create({ app: 'discover', path: '...' });
url.isReachable(); // true / false |
👆 it could use
|
This is a proposal to introduce
KibanaURL
interface/class intoshare
plugin.KibanaURL
interfaceSimilar to
URL
class in Node.js andURL
Web API theKibanaURL
interface would have the following fields.But it would also have Kibana specific fields, such as below.
.app
is a string representing the Kibana app ID, ifKibanaURL
is internal link into Kibana. Or empty string if link is external. The app ID could be extracted manually frompathname
, 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 usewindow.location.href
andwindow.open()
depending ifnewTab
flag is set..navigate()
is a method that navigates within Kibana in SPA (single page app) way. Kibana useshistory
andreact-router
packages in Core for navigation between apps.navigate()
method would leveragecore.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 inreact-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 contractcreateUrl
method will returnKibanaURL
instead ofstring
.createUrl
could still return a string but the generator could have another methodcreateUrlObject
(orcreateKibanaURL
) which would returnKibanaURL
, as suggested here.share
plugin will exposeKibanaURL
constructor, e.gnew plugins.share.KibanaURL({ ... })
, ornew plugins.share.URL({ ... })
, orplugins.share.createURL({ ... })
.Use cases
Better DX
One can encapsulate all information about internal Kibana URL into a single object.
KibanaURL
can easily create a fullKibanaURL
providing only partial information, defaults are used for the skipped fields:Developer has access to all fields, even the ones that were skipped when constructing
KibanaURL
.And developer has access to Kibana-specific functionality:
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: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 astring
. Instead that could beKibanaURL
.If we don't make it
KibanaURL
the alternative is to add 2 new fields to embeddable output—editApp
andeditPath
.cc @elastic/kibana-app-arch @stacey-gammon @flash1293
The text was updated successfully, but these errors were encountered: