-
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
[uiApp] replace uiApp.injectVars with server.injectUiAppVars() #17485
Changes from 1 commit
38204a6
d8a4765
c3c0fb2
d48662f
976b143
052d0be
2dd7abe
8298607
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`UiAppsMixin creates kbnServer.uiApps from uiExports 1`] = ` | ||
Array [ | ||
StubUiApp { | ||
"_hidden": true, | ||
"_id": "foo", | ||
}, | ||
StubUiApp { | ||
"_hidden": false, | ||
"_id": "bar", | ||
}, | ||
] | ||
`; | ||
|
||
exports[`UiAppsMixin decorates server with methods 1`] = ` | ||
Array [ | ||
Array [ | ||
"server", | ||
"getAllUiApps", | ||
[Function], | ||
], | ||
Array [ | ||
"server", | ||
"getUiAppById", | ||
[Function], | ||
], | ||
Array [ | ||
"server", | ||
"getHiddenUiAppById", | ||
[Function], | ||
], | ||
Array [ | ||
"server", | ||
"injectUiAppVars", | ||
[Function], | ||
], | ||
Array [ | ||
"server", | ||
"getInjectedUiAppVars", | ||
[Function], | ||
], | ||
] | ||
`; | ||
|
||
exports[`UiAppsMixin server.getAllUiApps() returns hidden and non-hidden apps 1`] = ` | ||
Array [ | ||
StubUiApp { | ||
"_hidden": true, | ||
"_id": "foo", | ||
}, | ||
StubUiApp { | ||
"_hidden": false, | ||
"_id": "bar", | ||
}, | ||
] | ||
`; | ||
|
||
exports[`UiAppsMixin server.getHiddenUiAppById() returns hidden apps when requested, undefined for non-hidden and unknown apps: hidden 1`] = ` | ||
StubUiApp { | ||
"_hidden": true, | ||
"_id": "foo", | ||
} | ||
`; | ||
|
||
exports[`UiAppsMixin server.getHiddenUiAppById() returns hidden apps when requested, undefined for non-hidden and unknown apps: non-hidden 1`] = `undefined`; | ||
|
||
exports[`UiAppsMixin server.getHiddenUiAppById() returns hidden apps when requested, undefined for non-hidden and unknown apps: unknown 1`] = `undefined`; | ||
|
||
exports[`UiAppsMixin server.getUiAppById() returns non-hidden apps when requested, undefined for non-hidden and unknown apps: hidden 1`] = `undefined`; | ||
|
||
exports[`UiAppsMixin server.getUiAppById() returns non-hidden apps when requested, undefined for non-hidden and unknown apps: non-hidden 1`] = ` | ||
StubUiApp { | ||
"_hidden": false, | ||
"_id": "bar", | ||
} | ||
`; | ||
|
||
exports[`UiAppsMixin server.getUiAppById() returns non-hidden apps when requested, undefined for non-hidden and unknown apps: unknown 1`] = `undefined`; | ||
|
||
exports[`UiAppsMixin server.injectUiAppVars()/server.getInjectedUiAppVars() stored injectVars provider and returns provider result when requested: bar 1`] = ` | ||
Object { | ||
"thisIsFoo": false, | ||
} | ||
`; | ||
|
||
exports[`UiAppsMixin server.injectUiAppVars()/server.getInjectedUiAppVars() stored injectVars provider and returns provider result when requested: foo 1`] = ` | ||
Object { | ||
"thisIsFoo": true, | ||
} | ||
`; | ||
|
||
exports[`UiAppsMixin server.injectUiAppVars()/server.getInjectedUiAppVars() stored injectVars provider and returns provider result when requested: unknown 1`] = `Object {}`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,4 +29,18 @@ export function uiAppsMixin(kbnServer, server) { | |
server.decorate('server', 'getAllUiApps', () => kbnServer.uiApps.slice(0)); | ||
server.decorate('server', 'getUiAppById', id => appsById.get(id)); | ||
server.decorate('server', 'getHiddenUiAppById', id => hiddenAppsById.get(id)); | ||
|
||
const injectedVarProviders = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question/issue: why is this an array and not a map, do you have a use case in mind when we want to register several different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, definitely intended to support multiple providers for the same appId so that plugins can inject vars into other applications. Down the road I intend to add support for a |
||
server.decorate('server', 'injectUiAppVars', function (id, provider) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: arrow function maybe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe we can use more descriptive name for |
||
injectedVarProviders.push({ id, provider, }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: we use trailing commas even if all properties are on the same line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have gone in to remove that like four times :P |
||
}); | ||
|
||
server.decorate('server', 'getInjectedUiAppVars', async (id) => { | ||
return await injectedVarProviders | ||
.filter(p => p.id === id) | ||
.reduce(async (acc, { provider }) => ({ | ||
...await acc, | ||
...await provider(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.
issue: it seems this
if
will never trigger sinceinjectVars
is always "trimmed" inui_export_types/ui_apps.js
now. If I'm not mistaken, we should do this check there instead?