-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Spaces mapping #18778
Spaces mapping #18778
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job, I really like the concept and implementation of the UrlContext
@@ -36,7 +37,9 @@ export const spaces = (kibana) => new kibana.Plugin({ | |||
mappings, | |||
home: ['plugins/spaces/register_feature'], | |||
injectDefaultVars: function () { | |||
return { }; | |||
return { | |||
spaces: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a better way to do this, but I need the spaces
var here so that angular can inject it here. The default isn't really used, but I make use of the override available here.
This allows the Space Selection screen to have all the information it needs to render, instead of having the client make another ajax call once the page loads. We'd end up with another loading screen while we waited for the Space Selector to load the available spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... so if we don't define it here we can't provide it via the render call? That's super weird...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - the Angular controller would actually fail because there isn't a "spacesProvider" registered with the system. At least that's what happened with my testing...I could be overlooking a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen crazier things, it works for me!
server.ext('onRequest', async function spacesOnRequestHandler(request, reply) { | ||
const path = request.path; | ||
|
||
// If navigating within the context of a space, then we store the Space's URL Cpntext on the request, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Cpntext
and rewrire
are misspelled
}; | ||
|
||
request.setUrl(newUrl); | ||
request.app._spaceUrlContext = spaceUrlContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we only using _spaceUrlContext
below? I've seen usages of Weakmap to do similar things without "leaking" the variable, perhaps we could do something similar here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, this is the only place it's used, but the Saved Objects Client will eventually need to have the space available to it. I haven't thought about what that will look like yet, but I can make it a Weakmap for now, and change it later if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could expose the UrlContext on the request itself, that way it's more generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we certainly could.
For the Saved Objects Client, I imagine that we'll be augmenting the reads/writes to include the new spaceId
property on the index mapping, so we will need a way to lookup the spaceId
from the URL Context. I'm worried that decoupling the URL Context from Spaces too much will make that translation confusing to follow.
I also thought about just storing the spaceId
instead, but that introduces a lot of unnecessary overhead on each request (because we have to look it up), even if the space id isn't needed for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR accomplishes the following:
URL Context
The URL Context is essentially the Space's display name, from the URL perspective. The URL Context is automatically derived from the Space's name, but can be manually adjusted by the user.
Example:
Space Name:
My Awesome Space
URL Context:
my-awesome-space
When using Kibana within this space, the URL will look similar to:
http://kibana.home/s/my-awesome-space/app/kibana
.Changing the URL Context of an existing space will effectively break existing links and bookmarks, so we display a warning message when you attempt to do this, but we don't currently prevent the change.
TODO