-
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
Add ContextService #41251
Add ContextService #41251
Conversation
2070364
to
521e320
Compare
This comment has been minimized.
This comment has been minimized.
💚 Build Succeeded |
Pinging @elastic/kibana-platform |
521e320
to
9d0f30b
Compare
💔 Build Failed |
retest |
💚 Build Succeeded |
9d0f30b
to
a762bbf
Compare
💚 Build Succeeded |
Still working on some of the updates requested here but have to go for the day, should have this updated tomorrow. |
a762bbf
to
bf822dc
Compare
💚 Build Succeeded |
068db26
to
7bb0177
Compare
💔 Build Failed |
retest |
💚 Build Succeeded |
ack: will review by the end of the week |
7bb0177
to
a62ceb9
Compare
💚 Build Succeeded |
docs/development/core/public/kibana-plugin-public.contextcontainer.md
Outdated
Show resolved
Hide resolved
i18n: I18nStart; | ||
uiSettings: UISettingsClientContract; | ||
} | ||
[contextName: string]: unknown; |
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.
is it a necessary declaration?
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.
Maybe not. I thought it could be useful to help show that the contextName
parameter in registerContext
corresponds to keys on this object.
this.contextNamesBySource = new Map<ContextSource, Array<keyof TContext>>([[coreId, []]]); | ||
} | ||
|
||
public registerContext = <TContextName extends keyof TContext>( |
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.
Can you elaborate on why we are using arrow functions here instead of class methods?
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.
Without using function properties, I was having issues with this
being the caller. I'll play around with this more to see if it's necessary.
@@ -17,7 +17,7 @@ | |||
* under the License. | |||
*/ | |||
|
|||
export function mapToObject<V = unknown>(map: Map<string, V>) { | |||
export function mapToObject<V = unknown>(map: ReadonlyMap<string, V>) { |
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.
Does this have any consequence in trying to pass in a Map?
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.
This function doesn't modify the argument and Map
is compatible with ReadonlyMap
so this change just makes this function more generally useful.
a62ceb9
to
d453b78
Compare
💚 Build Succeeded |
d453b78
to
c8e4070
Compare
docs/development/core/public/kibana-plugin-public.overlaystart.md
Outdated
Show resolved
Hide resolved
💚 Build Succeeded |
c8e4070
to
2a7261a
Compare
💚 Build Succeeded |
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.
Good start. Let's try API with real use cases for the application and HTTP services.
💔 Build Failed |
Summary
This adds a generic implementation for leveraging the Handler Context pattern.
This is a slightly updated version of the service proposed by #40554
Basic Example
Setting up a ContextContainer and configuring handlers
Invoking a configured handler
Details
This approach does leak details about consuming plugins to context service owners via the
plugin
argument that must be passed. We evaluated a few approaches and have decided that this had the least-bad consequences. If we decide that this level of detail is dangerous, we could go a step further and use some sort of opaque identifier such as a symbol generated by Core for each plugin.To do in future PRs:
Dev Docs
New Platform plugins may now implement their own context API on the server or client.
A
IContextContainer
can be used by any Core service or plugin (known as the "service owner") which wishes to expose APIs in a handler function. The container object will manage registering context providers and configuring a handler with all of the contexts that should be exposed to the handler's plugin. This is dependent on the dependencies that the handler's plugin declares.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers