-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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 to server #42395
Add ContextService to server #42395
Conversation
💔 Build Failed |
dfb5a90
to
0d62cd1
Compare
💔 Build Failed |
bd171bb
to
46226a6
Compare
💚 Build Succeeded |
Pinging @elastic/kibana-platform |
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.
Looks great!
💚 Build Succeeded |
* mandatory JSON manifest file. | ||
* @internal | ||
*/ | ||
export interface PluginManifest { |
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.
what is the main purpose of this file? as I understand it's to share types between client and server. PluginManifest
exists on the server-side only, doesn't 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.
Right I made it for sharing types, but I figured I'd place all types for the plugins service in here for consistency.
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 figured I'd place all types for the plugins service in here for consistency.
ok. Although I'd rather keep them closer to the usage place. It helps us to track unused types and dependencies between different modules/files.
@@ -18,7 +18,7 @@ | |||
*/ | |||
|
|||
import { ContextContainer } from './context'; | |||
import { PluginOpaqueId } from '../plugins'; | |||
import { PluginOpaqueId } from '../server'; |
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.
the problem of sharing server and client code popped up several times in this PR and in #39891
I'm wondering why we encourage domain-focus folder structure for plugins in favor of technology-focus but do not follow the same convention in the core? why not similarly structure core services?
- core
|__ context
|__ public
| |__ index.ts
|__ server
| |__ index.ts
|__ common/utils/ whatever you want
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.
That's not a bad idea. It could complicate some of the bundling / building logic, but I'd be interested in exploring that (in a separate PR / issue) if it helps alleviate this problem.
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.
could your create a separate issue to investigate this approach? we have to address problem of importing context
from utils
eventually.
1f36cab
to
951d9bf
Compare
💔 Build Failed |
d804c75
to
bd4cd89
Compare
💚 Build Succeeded |
Removing dev_docs label to avoid duplicating the docs in #41251 |
Summary
This ports the ContextService (#41251) over to the Server side as well. No significant differences here, though the changes to the server-side PluginsService is slightly different, due to the way the backend does plugin discovery.
I split this into two commits.
utils
directory. This still doesn't feel like quite the right place, so I'm open to suggestions on this.