[uiSettings] support defining settings with uiExports #12250

spalger · 2017-06-09T03:50:24Z

This PR introduces a new uiExports type, uiSettingDefaults. The specification for these uiExports follow the same pattern as the defaults did before this change, the only difference is that they can be defined in a plugin's definition file and will be merged with the uiSettingDefaults from all other plugins.

Summary:

define a uiExports.uiSettingDefaults key your plugin definition to add uiSettingDefaults
uiSettingDefaults must be unique. If a key is already defined elsewhere then the server will not start.
defined settings will display in the advanced settings editor, along with the intended default value and description
the REST API is identical to before
the browser API is identical to before
defaults for timelion have been extracted to the timelion plugin definition and the existing defaults are defined in the kibana plugin definition

PS: I'd like to split the defaults in the kibana plugin from the "core" defaults and define those in ui/ui_settings, but didn't want to make too many changes to the defaults list in this PR. If you're up for the change now let me know, or I will send another PR once this one is merged.

Release Note: We introduced a new uiExports type, uiSettingDefaults. The specification for these uiExports follow the same pattern as the defaults did before this change, the only difference is that they can be defined in a plugin's definition file and will be merged with the uiSettingDefaults from all other plugins.

spalger · 2017-06-13T03:18:51Z

cc @tsullivan

azasypkin

LGTM, just few tiny nits.

azasypkin · 2017-06-13T13:16:42Z

src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js

+        foo: 'bar'
+      });
+      sinon.assert.calledOnce(uiSettingsServiceFactory);
+      const { args } = uiSettingsServiceFactory.firstCall;


nit: how about this instead?

sinon.assert.calledWithExactly(uiSettingsServiceFactory, server, { foo: 'bar', getDefaults: sinon.match.func });

azasypkin · 2017-06-13T13:39:33Z

src/ui/ui_settings/ui_exports_consumer.js

@@ -0,0 +1,24 @@
+export class UiExportsConsumer {


question: Would you mind adding a brief comment for this class (what this is supposed to do and what are these consumers)? Honestly, I'm having a hard time trying to understand what this thing does :/

azasypkin · 2017-06-13T13:53:13Z

src/ui/ui_settings/ui_settings_service.js

@@ -1,8 +1,6 @@
 import { defaultsDeep, noop } from 'lodash';
 import { errors as esErrors } from 'elasticsearch';

-import { getDefaultSettings } from './defaults';
-
 function hydrateUserSettings(userSettings) {
  return Object.keys(userSettings)
    .map(key => ({ key, userValue: userSettings[key] }))


Not related to this PR, but looks funny that we map before filter that theoretically results in allocation of unnecessary objects, wondering if there is any hidden rationale here :)

IIRC I was coming down from an rx/observable high and didn't like that this method was previously implemented as a reduce that just assigned to a value and returned it (so that it could filter that value)... Then I remember wanting to implement it without the if()... Yeah, not a huge fan of how it turned out. If you want to take a stab at it in another pr I'd love to see your prefered alternative

azasypkin · 2017-06-13T14:36:40Z

src/ui/ui_settings/ui_settings_service_for_request.js

+ *  @property {AsyncFunction} [options.getDefaults] async function that returns defaults/details about
+ *                            the uiSettings.
+ *  @property {AsyncFunction} [options.readInterceptor] async function that is called when the
+ *                            UiSettingsService does a read() an has an oportunity to intercept the


typo: read() an has -----> read() and has?

kimjoar

I'm struggling with following the flow here, but it feels like it's unavoidable without larger changes.

LGTM, just had a couple questions/nits.

kimjoar · 2017-06-13T14:59:25Z

src/core_plugins/tests_bundle/index.js

@@ -57,7 +56,9 @@ export default (kibana) => {
          });
        }

-        env.defaultUiSettings = getDefaultSettings();
+        env.defaultUiSettings = plugins.kbnServer.uiExports.consumers
+          .find(consumer => consumer.getUiSettingDefaults)


I'm a bit slow today, and I just couldn't understand this at first. I was just thinking "waaait, so getUiSettingDefaults is not a function"? But yeah, I understand it now. I'd love a comment on why it's only doing this on the first consumer it finds that has this function.

I also think this should change to something like:

.find(consumer => consumer.getUiSettingDefaults !== undefined)

or

.find(consumer => isFunction(consumer.getUiSettingDefaults))

kimjoar · 2017-06-13T15:07:04Z

src/ui/index.js


 export default async (kbnServer, server, config) => {
  const uiExports = kbnServer.uiExports = new UiExports({
    urlBasePath: config.get('server.basePath')
  });

+  await kbnServer.mixin(uiSettingsMixin);


I'm soooo looking forward to 🔥 these mixins.

Why is this moved here, btw?

Moved here so that the uiExport consumer is registered before we try to consume all of the exports. This bit of pluggability isn't quite as pluggable as originally thought, uiExportConsumers need to be registered between the time that kbnServer.uiExports is created and when kbnServer.uiExports.consumePlugin() is called for the first time, which doesn't provide any time in this case...

kimjoar · 2017-06-13T15:11:56Z

src/ui/ui_settings/ui_settings_mixin.js

-    return uiSettingsServiceFactory(server, options);
+  server.decorate('server', 'uiSettingsServiceFactory', (options = {}) => {
+    return uiSettingsServiceFactory(server, {
+      getDefaults,


Why pass around the function instead of just the defaults themselves?

So the defaults can change without needing to tell the uiSettingsService, but if you're really against it I can change that.

Ah 👍 Let's keep it (maybe add a comment above it, so it's easier to understand when looking at it later and thinking the same thought I had?)

…i-exports

* [uiSettings] add `uiSettingDefaults` export type * [uiSettings/uiExportsConsumer] ensure that there are not conflicts * use `sinon.assert.calledWithExactly()` * describe the UiExportsConsumer class * make uiExport consumers filtering intention more clear * fix typo * fix typos * add note about why getDefaults() is a function (cherry picked from commit 7bec60c)

* [uiSettings] make service request based (#12243) * [server/uiSettings] make uiSettings service request based * [server/uiSettings] disambiguate UiSettings/Service * [server/uiSettings] link to PR in removal error * [server/uiSettings] await _read before hydrating * [server/uiSettings] focus tests, remove server integration * [server/uiSettings] add tests for readInterceptor() arg * [server/uiSettings] add server integration tests * [server/uiExports] fix replaceInjectedVars tests * [server/uiSettings] convert all methods to use async/await * [uiSettings/serviceFactory] fix doc block * [uiSettings/service] fix doc block * [uiSettings/tests/callClusterStub] stop tracking state needlessly * [uiSettings/tests] remove invalid tests and pointless promise helpers * [uiSettings/forRequest] fix typo * [uiSettings/tests] remove mixture of arrow and function expressions * [uiSettings/tests/callClusterStub] leverage sinon.calledWithExactly * [uiSettings/mixin/tests] add exception for eslint import/no-duplicates * [uiSettings/mixin/tests] wrap single args in parens (cherry picked from commit 65d6b5d) * [uiSettings] support defining settings with uiExports (#12250) * [uiSettings] add `uiSettingDefaults` export type * [uiSettings/uiExportsConsumer] ensure that there are not conflicts * use `sinon.assert.calledWithExactly()` * describe the UiExportsConsumer class * make uiExport consumers filtering intention more clear * fix typo * fix typos * add note about why getDefaults() is a function (cherry picked from commit 7bec60c)

spalger added v6.0.0 WIP Work in progress labels Jun 9, 2017

spalger force-pushed the ui-settings/ui-exports branch 2 times, most recently from f5970ce to 3eadde0 Compare June 12, 2017 21:22

[uiSettings] add uiSettingDefaults export type

eec0833

spalger force-pushed the ui-settings/ui-exports branch from 3eadde0 to eec0833 Compare June 12, 2017 21:27

[uiSettings/uiExportsConsumer] ensure that there are not conflicts

6a843b3

spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc review and removed WIP Work in progress labels Jun 12, 2017

spalger requested review from azasypkin and kimjoar June 12, 2017 23:12

azasypkin approved these changes Jun 13, 2017

View reviewed changes

kimjoar approved these changes Jun 13, 2017

View reviewed changes

spalger added 8 commits June 13, 2017 11:10

Merge branch 'master' of github.com:elastic/kibana into ui-settings/u…

28cd4e0

…i-exports

use sinon.assert.calledWithExactly()

8a57c2c

describe the UiExportsConsumer class

725471c

make uiExport consumers filtering intention more clear

603b888

fix typo

d99b855

fix typos

51a4c73

add note about why getDefaults() is a function

a2e108f

Merge branch 'master' of github.com:elastic/kibana into ui-settings/u…

a124132

…i-exports

spalger merged commit 7bec60c into elastic:master Jun 13, 2017

spalger mentioned this pull request Jun 28, 2017

[5.x] Backport #12243 and #12250 #12531

Merged

jimgoodwin added the release_note:enhancement label Jul 25, 2017

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

spalger deleted the ui-settings/ui-exports branch October 18, 2019 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[uiSettings] support defining settings with uiExports #12250

[uiSettings] support defining settings with uiExports #12250

spalger commented Jun 9, 2017 •

edited by jimgoodwin

Loading

spalger commented Jun 13, 2017

azasypkin left a comment

azasypkin Jun 13, 2017

azasypkin Jun 13, 2017

azasypkin Jun 13, 2017

spalger Jun 13, 2017

azasypkin Jun 13, 2017

kimjoar left a comment

kimjoar Jun 13, 2017

kimjoar Jun 13, 2017

spalger Jun 13, 2017

kimjoar Jun 13, 2017

kimjoar Jun 13, 2017

spalger Jun 13, 2017

kimjoar Jun 13, 2017

[uiSettings] support defining settings with uiExports #12250

[uiSettings] support defining settings with uiExports #12250

Conversation

spalger commented Jun 9, 2017 • edited by jimgoodwin Loading

spalger commented Jun 13, 2017

azasypkin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimjoar left a 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

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 commented Jun 9, 2017 •

edited by jimgoodwin

Loading