-
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
Allows selected injected default vars to be merged instead of overwritten #25318
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.
LGTM - I did a quick audit of our existing default vars, and I'm not seeing any overlap currently that would be relying on the old behavior, and if they wanted to use the old behavior, they can do so using replaceInjectedVars
.
I assume we will also want to use // kibana.js
uiExports: {
app: {
// ...
injectedVars() {
return {
uiCapabilities: {
x: true,
}
}
}
}
}
// nav_control.js
uiExports: {
navControl: [ /* ... */ ],
defaultInjectedVars() {
return {
uiCapabilities: {
y: true,
}
}
}
} |
Actually, I take that back, I think we should limit the deep merging to just the |
💚 Build Succeeded |
If we're going to handle the uiCapabilities in a special case, should we just do a "flat merge" there as well? So it becomes a barely deep only for uiCapabilities merge? |
I'm good with that if it accomplishes what we need |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
28747ab
to
0021f89
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.
LGTM
0021f89
to
70bb471
Compare
💔 Build Failed |
70bb471
to
b29db21
Compare
💚 Build Succeeded |
This reverts commit b29db21.
💔 Build Failed |
retest |
💚 Build Succeeded |
…tten (elastic#25318) ## Summary Currently, injected variables (via `injectDefaultVars`) allows a variable from one plugin to overwrite a variable of the same name that another plugin provided. This PR updates the collection of default variables to "flat merge" select variables together, so that properties from one plugin don't clobber variables from another plugin. This is required to implement the `uiCapabilities` collection that we discussed. ### Example (current functionality): ```javascript // Plugin 1 injectDefaultVars() { return { a: 'plugin 1', b: { c: 'plugin 1' } } } ``` ```javascript // Plugin 2 injectDefaultVars() { return { b: { d: 'plugin 2' } } } ``` Results in: ```javascript { a: 'plugin 1', b: { d: 'plugin 2' } } ``` ### Example (after this PR): ```javascript // Plugin 1 injectDefaultVars() { return { a: 'plugin 1', b: { c: 'plugin 1' } } } ``` ```javascript // Plugin 2 injectDefaultVars() { return { b: { d: 'plugin 2' } } } ``` Results in: ```javascript { a: 'plugin 1', b: { c: 'plugin 1', d: 'plugin 2' } } ```
Summary
Currently, injected variables (via
injectDefaultVars
) allows a variable from one plugin to overwrite a variable of the same name that another plugin provided.This PR updates the collection of default variables to "flat merge" select variables together, so that properties from one plugin don't clobber variables from another plugin.
This is required to implement the
uiCapabilities
collection that we discussed.Example (current functionality):
Results in:
Example (after this PR):
Results in: