[uiApp] replace uiApp.injectVars with server.injectUiAppVars() #17485

spalger · 2018-03-30T22:47:05Z

With #17474 we will be moving the configuration for uiApp specs to the browser which means they can no longer include functions like injectVars() which expect to be run on the server. Instead we need to expose hooks into the ui_render module that provide similar functionality.

The new API is:

server.injectUiAppVars('kibana', () => ({
  someConfigVar: server.config().get('kibana.someConfigVar')
}))

Long term we will probably want to get rid of this API entirely, perhaps replacing most existing uses with a static map of configuration values from kibana.yml that should be injected into a uiPlugin, but that requires more planning and thought than I want to put into this at this time.

elasticmachine · 2018-03-31T00:20:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-04T14:23:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-04-05T13:41:32Z

src/ui/ui_apps/ui_apps_mixin.js

@@ -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 = [];
+  server.decorate('server', 'injectUiAppVars', function (id, provider) {


nit: arrow function maybe?

azasypkin · 2018-04-05T13:42:07Z

src/ui/ui_apps/ui_apps_mixin.js

+
+  const injectedVarProviders = [];
+  server.decorate('server', 'injectUiAppVars', function (id, provider) {
+    injectedVarProviders.push({ id, provider, });


question: we use trailing commas even if all properties are on the same line?

I have gone in to remove that like four times :P

azasypkin · 2018-04-05T13:44:00Z

src/ui/ui_apps/ui_apps_mixin.js

@@ -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 = [];
+  server.decorate('server', 'injectUiAppVars', function (id, provider) {


nit: maybe we can use more descriptive name for id here and in the function below? Like appId/pluginId or something.

azasypkin · 2018-04-05T13:46:32Z

src/ui/ui_apps/ui_apps_mixin.js

@@ -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 = [];


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 provider's for the same id? If no, I'd use map and even throw if someone tries to register provider with the same name twice. If yes, then we need tests for this use case.

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 * appId that will allow us to remove most if not all instances of uiExports.injectDefaultVars and uiExports.replaceInjectedVars, but didn't want to muddy up this pr.

azasypkin · 2018-04-05T13:55:00Z

src/ui/ui_apps/ui_apps_mixin.test.js

+import { uiAppsMixin } from './ui_apps_mixin';
+
+jest.mock('./ui_app', () => ({
+  UiApp: class StubUiApp {


note: Heh, I'm surprised that having a class declaration within a property declaration is a valid syntax. There is no need to change anything here, I'm just old fashioned and grumbling a bit :)

azasypkin · 2018-04-05T14:00:33Z

src/ui/ui_apps/ui_apps_mixin.test.js

+
+  describe('server.getUiAppById()', () => {
+    it('returns non-hidden apps when requested, undefined for non-hidden and unknown apps', () => {
+      expect(server.getUiAppById('foo')).toMatchSnapshot('hidden');


optional nit: I don't have a strong opinion on that so up to you, but using snapshot just for undefined (here and in test below) sounds like an overkill to me.

🤷‍♀️ I like consistency, but I'm not married to it

Nit: I think this should just be inlined as undefined. Same with the {} ones. I think we should optimize for readability, not consistency.

azasypkin · 2018-04-05T14:50:44Z

src/ui/ui_apps/ui_app.js

@@ -22,6 +21,10 @@ export class UiApp {
      throw new Error('Every app must specify an id');
    }

+    if (spec.injectVars) {


issue: it seems this if will never trigger since injectVars is always "trimmed" in ui_export_types/ui_apps.js now. If I'm not mistaken, we should do this check there instead?

azasypkin

LGTM, just a couple of minor things

…injectedVars

elasticmachine · 2018-04-05T19:05:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kimjoar · 2018-04-06T15:09:13Z

src/ui/ui_apps/ui_apps_mixin.test.js

+
+  describe('server.getUiAppById()', () => {
+    it('returns non-hidden apps when requested, undefined for non-hidden and unknown apps', () => {
+      expect(server.getUiAppById('foo')).toMatchSnapshot('hidden');


Nit: I think this should just be inlined as undefined. Same with the {} ones. I think we should optimize for readability, not consistency.

…injectedVars

elasticmachine · 2018-04-06T22:24:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ic#17485) * [uiApp] replace uiApp.injectVars with server.injectUiAppVars() * [server/injectUiAppVars] cleanup styling * [server/injectUiAppVars] add test to verify injectUiAppVars merging * [server/injectUiAppVars] describe what type of id we expect * [uiExportTypes/uiApp] add removal error to proper location * [uiApps/tests] avoid snapshots for undefined/{} values

… (#17601) * [uiApp] replace uiApp.injectVars with server.injectUiAppVars() * [server/injectUiAppVars] cleanup styling * [server/injectUiAppVars] add test to verify injectUiAppVars merging * [server/injectUiAppVars] describe what type of id we expect * [uiExportTypes/uiApp] add removal error to proper location * [uiApps/tests] avoid snapshots for undefined/{} values

spalger · 2018-04-07T00:19:49Z

6.3/6.x: cd54326

spalger added WIP Work in progress Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.3.0 labels Mar 30, 2018

spalger force-pushed the remove/uiApp/injectedVars branch 3 times, most recently from 8a2f4aa to 09a262c Compare March 30, 2018 23:28

[uiApp] replace uiApp.injectVars with server.injectUiAppVars()

38204a6

spalger force-pushed the remove/uiApp/injectedVars branch from 09a262c to 38204a6 Compare April 4, 2018 13:12

spalger requested review from kimjoar and azasypkin April 4, 2018 13:17

spalger added review and removed WIP Work in progress labels Apr 4, 2018

spalger mentioned this pull request Apr 4, 2018

[new-platform] implement core.applicationService #17474

Closed

azasypkin reviewed Apr 5, 2018

View reviewed changes

azasypkin approved these changes Apr 5, 2018

View reviewed changes

spalger added 5 commits April 5, 2018 10:53

Merge branch 'master' of github.com:elastic/kibana into remove/uiApp/…

d8a4765

…injectedVars

[server/injectUiAppVars] cleanup styling

c3c0fb2

[server/injectUiAppVars] add test to verify injectUiAppVars merging

d48662f

[server/injectUiAppVars] describe what type of id we expect

976b143

[uiExportTypes/uiApp] add removal error to proper location

052d0be

kimjoar approved these changes Apr 6, 2018

View reviewed changes

spalger added 2 commits April 6, 2018 14:02

Merge branch 'master' of github.com:elastic/kibana into remove/uiApp/…

2dd7abe

…injectedVars

[uiApps/tests] avoid snapshots for undefined/{} values

8298607

spalger merged commit 1ecddb4 into elastic:master Apr 6, 2018

spalger mentioned this pull request Apr 6, 2018

[6.x] [uiApp] replace uiApp.injectVars with server.injectUiAppVars() (#17485) #17601

Merged

spalger deleted the remove/uiApp/injectedVars branch April 7, 2018 00:19

spalger mentioned this pull request May 7, 2018

[new-platform] UI plugins #18874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[uiApp] replace uiApp.injectVars with server.injectUiAppVars() #17485

[uiApp] replace uiApp.injectVars with server.injectUiAppVars() #17485

spalger commented Mar 30, 2018 •

edited

Loading

elasticmachine commented Mar 31, 2018

elasticmachine commented Apr 4, 2018

azasypkin Apr 5, 2018

azasypkin Apr 5, 2018

spalger Apr 5, 2018

azasypkin Apr 5, 2018

azasypkin Apr 5, 2018

spalger Apr 5, 2018 •

edited

Loading

azasypkin Apr 5, 2018

azasypkin Apr 5, 2018

spalger Apr 5, 2018 •

edited

Loading

kimjoar Apr 6, 2018

azasypkin Apr 5, 2018

azasypkin left a comment

elasticmachine commented Apr 5, 2018

kimjoar Apr 6, 2018

elasticmachine commented Apr 6, 2018

spalger commented Apr 7, 2018

[uiApp] replace uiApp.injectVars with server.injectUiAppVars() #17485

[uiApp] replace uiApp.injectVars with server.injectUiAppVars() #17485

Conversation

spalger commented Mar 30, 2018 • edited Loading

elasticmachine commented Mar 31, 2018

💚 Build Succeeded

elasticmachine commented Apr 4, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 5, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented Apr 6, 2018

💚 Build Succeeded

spalger commented Apr 7, 2018

spalger commented Mar 30, 2018 •

edited

Loading

spalger Apr 5, 2018 •

edited

Loading

spalger Apr 5, 2018 •

edited

Loading