Expose whitelisted config values to client-side plugin #50641

pgayvallet · 2019-11-14T12:55:32Z

Summary

Fixes #41990

PR adds a mechanism to share some explicitly whitelisted configuration keys to the client part of the plugin.
The configuration access in the client follow the API already implemented server-side, using something like

const config = this.initializerContext.config.get<ConfigType>();

The values whitelisting follows the consensus in #41990:

const configSchema = schema.object({
  secret: schema.string({ defaultValue: 'Not really a secret :/' }),
  uiProp: schema.string({ defaultValue: 'Accessible from client' }),
});

type ConfigType = TypeOf<typeof configSchema>;

export const config: PluginConfigDescriptor<ConfigType> = {
  exposeToBrowser: {
    uiProp: true,
  },
  schema: configSchema,
};

The actual server->client sharing is based on the injectedMetadata, as we currently don't have a proper 'NP' way to share things between the two world.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

Dev Docs

New Platform plugins with both a server and client parts can now expose configuration properties to the client-side plugin.

The properties to expose must be whitelisted in the config declaration

// my_plugin/server/index.ts
const configSchema = schema.object({
  secret: schema.string({ defaultValue: 'Not really a secret :/' }),
  uiProp: schema.string({ defaultValue: 'Accessible from client' }),
});

type ConfigType = TypeOf<typeof configSchema>;

export const config: PluginConfigDescriptor<ConfigType> = {
  exposeToBrowser: {
    uiProp: true,
  },
  schema: configSchema,
};

And can then be accessed in the client-side plugin using the PluginInitializerContext:

// my_plugin/public/index.ts
interface ClientConfigType {
  uiProp: string;
}
export class Plugin implements Plugin<PluginSetup, PluginStart> {
  constructor(private readonly initializerContext: PluginInitializerContext) {}
  public async setup(core: CoreSetup, deps: {}) {
    const config = this.initializerContext.config.get<ClientConfigType>();
    // ...
  }

elasticmachine · 2019-11-14T12:55:35Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2019-11-14T13:03:51Z

src/core/server/plugins/plugins_service.ts

+    const uiPlugins = this.pluginsSystem.uiPlugins();
    return {
-      contracts: await this.pluginsSystem.setupPlugins(deps),
-      uiPlugins: this.pluginsSystem.uiPlugins(),
+      contracts,
+      uiPlugins: {
+        ...uiPlugins,
+        config: this.generateUiPluginsConfigs(uiPlugins.public),
+      },


uiPlugins are directly serialized in injected metadatas, so I added the configuration in a distinct property instead of trying to merge in the uiPlugins structs

pgayvallet · 2019-11-14T13:06:12Z

src/core/server/plugins/plugins_service.ts

+  private generateUiPluginsConfigs(
+    uiPlugins: Map<string, DiscoveredPlugin>
+  ): Map<PluginName, Observable<unknown>> {


The observable is currently not of use, as we take(1) before serializing to InjectedMetadata, however this is futur-proof for when we'll want to implement dynamic client-side config refresh as we do on server side.

pgayvallet · 2019-11-14T13:09:08Z

src/core/server/plugins/types.ts

+export interface PluginConfigDescriptor<T = any> {
+  exposeToBrowser?: Array<keyof T>;
+  schema: PluginConfigSchema<T>;
+}


The <T =any> here instead of unknow allows the plugins to use it either with or without specifying the type

export const config: PluginConfigDescriptor = { exposeToBrowser: ['uiProp'], schema: configSchema, }; export const config: PluginConfigDescriptor<ConfigType> = { exposeToBrowser: ['uiProp'], schema: configSchema, };

with default unknown type, the exposeToBrowser?: Array<keyof T>; will be an Array<never>, forcing to actually specify the type when declaring the PluginConfigDescriptor

pgayvallet · 2019-11-14T13:10:47Z

src/plugins/testbed/public/plugin.ts

 export class TestbedPlugin implements Plugin<TestbedPluginSetup, TestbedPluginStart> {
-  public setup(core: CoreSetup, deps: {}) {
+  constructor(private readonly initializerContext: PluginInitializerContext) {}
+
+  public async setup(core: CoreSetup, deps: {}) {
+    const config = await this.initializerContext.config
+      .create<ConfigType>()
+      .pipe(take(1))
+      .toPromise();
+
    // eslint-disable-next-line no-console
-    console.log(`Testbed plugin set up`);
+    console.log(`Testbed plugin set up. uiProp: '${config.uiProp}'`);


I added an example usage in the testbed plugin. The commit is isolated, tell me if I should remove it.

elasticmachine · 2019-11-14T14:08:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0df8a6f

mshustov · 2019-11-18T10:13:59Z

src/core/server/plugins/plugins_service.ts

+              ),
+          ];
+        }
+        return [pluginId, of({})];


is this necessary with your check here? daf09e3#diff-ce5eebb41b6315c38446e8a68a4ca0aeR242

I was filtering null values after the map call using [...uiPlugins].map([...]).filter, but it seems typescript doesn't properly recognize Observable<unknown> | null filtered with !== null as Observable<unknown>. So the only purpose of this is to avoid having the method return type to Map<PluginName, Observable<unknown> | null>. But this can be changed

mshustov · 2019-11-18T10:16:48Z

src/core/server/plugins/plugins_service.ts

+            this.configService
+              .atPath(plugin.configPath)
+              .pipe(
+                map((config: any) => pick(config || {}, configDescriptor.exposeToBrowser || []))


nit: exposeToBrowser cannot be falsy. daf09e3#diff-725c498a267472754f01cff2baa3901bR130

mshustov · 2019-11-18T10:18:24Z

src/core/server/plugins/plugins_service.ts

+      contracts,
+      uiPlugins: {
+        ...uiPlugins,
+        config: this.generateUiPluginsConfigs(uiPlugins.public),


optional: The name collision probability is minimal. But to simplify reading, shouldn't we handle it separately?

{ contracts, uiPlugins, uiConfig }

Yea, wasn't sure which one was the best approach. Both works with me, I can change it.

a87b7b0 separates configs from plugins

mshustov · 2019-11-18T10:26:49Z

src/core/public/plugins/plugin_context.ts

 ): PluginInitializerContext {
  return {
    opaqueId,
    env: coreContext.env,
+    config: {
+      create<T>() {
+        return of<T>((pluginConfig as unknown) as T);


Are we going to support runtime config updates on the client-side? Client-side plugins have a shorter time of life comparing to server-side ones. Should we provide a sync getter interface to simplify consumers' logic?

This is an excellent question.

There are two reasons for this:

Somewhere in the initial ticket I think I read that as NP was going to be SPA, the actual client app reload frequency was going to drastically decrease, and that we might want to support client-side runtime updates at some point, so this futur-proofs the API.

It's consistent with the server counterpart.

However currently, this returns an 'sync' of() observable, so a sync getter can easily be added or even replace the #create function. Drawback is that if at some point we actually implements client config reload, we'll have to remove this sync getter.

Not sure how realistic implementing the client config reload is in short/mid term, so I don't know what is best here.

Somewhere in the initial ticket I think I read that as NP was going to be SPA, the actual client app reload frequency was going to drastically decrease, and that we might want to support client-side runtime updates at some point, so this futur-proofs the API.

I'm not opposed to the idea. Albeit I see it is as a reasonable compromise, that platform sacrifice this functionality for the sake of plugin API simplicity. The platform can support config updates requesting a user to reload a page. We already do this for uiSettings.
https://github.com/elastic/kibana/blob/master/src/core/server/ui_settings/types.ts#L102
It reminds us that not all the parameters can be updated run time.
The main use case for ui config is myplugin.ui.enabled. It's can be updated in runtime as long as plugins use it with stateless API, e.g. to render content conditionally in React. Things get complicated when it's used with stateful API, like register a section on the management page or application registration.

Also, we can provide a sync config getter now and expose config stream when and if needed.

Ok, sounds good to me. Will change for sync getter.

mshustov · 2019-11-18T10:55:04Z

src/plugins/testbed/public/plugin.ts

+import { take } from 'rxjs/operators';
+import { Plugin, CoreSetup, PluginInitializerContext } from 'kibana/public';
+
+interface ConfigType {


👍 for simplicity.
optional: We can add a test that server side types can be reused.
server/config.ts

import { PluginConfigDescriptor } from 'kibana/server'; import { schema, TypeOf } from '@kbn/config-schema'; const configSchema = schema.object({ secret: schema.string({ defaultValue: 'Not really a secret :/' }), uiProp: schema.string({ defaultValue: 'Accessible from client' }), }); export const config: PluginConfigDescriptor<ConfigType> = { exposeToBrowser: ['uiProp'], schema: configSchema, }; export type ConfigType = TypeOf<typeof configSchema>; export type PublicConfigType = Pick<ConfigType, 'uiProp'>;

server/index.ts

import { config, ConfigType } from './config'; export { config };

public/plugin.ts

import { PublicConfigType } from '../server/config'; // or re-export it from /server/types.ts

This is great and ensure the PublicConfigType is correctly typed. Only question I have is regarding server-to-client imports: from what I understood, we needs to be extra careful on these imports to avoid polluting the client bundles with imports dependencies, so what should be the recommended way to do this? creates a types.ts files at the server plugin root folder level?

creates a types.ts files at the server plugin root folder level?

sounds reasonable. @kbn/config-schema is used in node env., so we cannot recommend declaring types under public or common.

…config-values

elasticmachine · 2019-11-18T16:05:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 95f5fc4

elasticmachine · 2019-11-19T18:16:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 1fe445f

joshdover · 2019-11-19T19:00:58Z

src/core/public/plugins/plugins_service.ts

@@ -76,7 +74,7 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
        new PluginWrapper(
          plugin,
          opaqueIds.get(id)!,
-          createPluginInitializerContext(this.coreContext, opaqueIds.get(id)!, plugin)
+          createPluginInitializerContext(this.coreContext, opaqueIds.get(id)!, plugin, config || {})


optional: use a default in the object destructure on line 64 instead:

plugins.forEach(({ id, plugin, config = {} }) => {

joshdover · 2019-11-19T19:29:29Z

src/core/server/plugins/types.ts

+  /**
+   * List of configuration properties that will be available on the client-side plugin.
+   */
+  exposeToBrowser?: Array<keyof T>;


Though we're only supporting top-level keys at this time, I think it would be more future proof if we type this as an object of string-boolean pairs. This could be extended in the future to support nested keys.

exposeToBrowser?: { [P in keyof T]?: boolean };

Plugins would then specify this like:

export const config: PluginConfigDescriptor<ConfigType> = { exposeToBrowser: { uiProp: true, }, schema: configSchema, };

To clarify, I think { [P in keyof T]?: boolean } is a better type to use now over Array<keyof T> because it is compatible with a type that we could use in the future for nested keys:

type BooleanValues<T> = { [P in keyof T]?: T[P] extends object ? boolean | BooleanValues<T> : boolean }

By using a compatible type now, we can easily add nested key support in the future without breaking all existing exposeToBrowser configurations.

joshdover · 2019-11-19T19:35:39Z

src/core/server/plugins/plugins_service.ts

+            configDescriptor &&
+            configDescriptor.exposeToBrowser &&
+            configDescriptor.exposeToBrowser.length > 0


Since we upgraded to TypeScript 3.7, I think you can use optional chaining here:

configDescriptor?.exposedToBrowser?.length > 0

I tried to, but the ?? operator was not properly reconized, both by eslint and script/typecheck, so I went back to the 2000 approach

…config-values

elasticmachine · 2019-11-20T09:37:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b147018

mshustov · 2019-11-20T17:30:18Z

docs/development/core/server/kibana-plugin-server.pluginconfigdescriptor.md

+type ConfigType = TypeOf<typeof configSchema>;
+
+export const config: PluginConfigDescriptor<ConfigType> = {
+  exposeToBrowser: ['uiProp'],


outdated docs?

Good catch, outdated tsdoc, thanks

mshustov · 2019-11-20T17:35:01Z

src/core/public/plugins/plugin_context.ts

 ): PluginInitializerContext {
  return {
    opaqueId,
    env: coreContext.env,
+    config: {
+      get<T>() {


T extends object? at least to outline the default value and requirements to the generic type

mshustov · 2019-11-20T17:37:35Z

src/core/server/plugins/types.ts

+ *
+ * @public
+ */
+export type PluginConfigSchema<T = unknown> = Type<T>;


I think = unknown is default value

…config-values

elasticmachine · 2019-11-21T11:19:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 24575b3

* introduce PluginConfigDescriptor type * inject client plugin configs in injectedMetadata * expose client config in PluginInitializerContext * add example implementation in testbed * update generated doc * only generates ui config entry for plugins exposing properties to client * separate plugin configs from plugins * restructure plugin services tests * fix test/mocks due to plugin configs api changes * add unit tests * update migration guide * update tsdoc * fix typecheck * use sync getter for config on client side instead of observable * change type of exposeToBrowser prop * updates generated doc * fix doc and address nits

eliperelman · 2019-11-23T21:37:32Z

@pgayvallet since this is a new feature, can we switch the release note to dev docs and add the relevant documentation?

pgayvallet · 2019-11-24T17:12:51Z

@eliperelman Indeed we should. Just added the Dev Docs section. Do you mind taking a quick look to check if that seems ok?

elasticmachine · 2019-11-24T18:48:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 24575b3

pgayvallet added 5 commits November 14, 2019 07:32

introduce PluginConfigDescriptor type

fb2b28c

inject client plugin configs in injectedMetadata

daf09e3

expose client config in PluginInitializerContext

ac9d19b

add example implementation in testbed

99790dd

update generated doc

0df8a6f

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Nov 14, 2019

pgayvallet added this to Code review in kibana-core [DEPRECATED] via automation Nov 14, 2019

pgayvallet changed the title ~~Expose whitelisted config values to frontend~~ Expose whitelisted config values to client-side plugin Nov 14, 2019

pgayvallet commented Nov 14, 2019

View reviewed changes

pgayvallet requested review from joshdover and mshustov November 14, 2019 13:11

dgieselaar mentioned this pull request Nov 18, 2019

[APM] New platform migration: replace injected vars #44232

Closed

mshustov reviewed Nov 18, 2019

View reviewed changes

pgayvallet added 5 commits November 18, 2019 14:04

Merge remote-tracking branch 'upstream/master' into kbn-41990-client-…

db25021

…config-values

only generates ui config entry for plugins exposing properties to client

9b93964

separate plugin configs from plugins

a87b7b0

restructure plugin services tests

b6d9a83

fix test/mocks due to plugin configs api changes

95f5fc4

pgayvallet added 4 commits November 18, 2019 20:20

add unit tests

e083125

update migration guide

c3c926a

update tsdoc

93ba773

fix typecheck

bd1e3bc

This comment has been minimized.

Sign in to view

joshdover reviewed Nov 19, 2019

View reviewed changes

pgayvallet added 3 commits November 20, 2019 08:46

change type of exposeToBrowser prop

5e34e91

Merge remote-tracking branch 'upstream/master' into kbn-41990-client-…

ebfc428

…config-values

updates generated doc

b147018

mshustov reviewed Nov 20, 2019

View reviewed changes

mshustov approved these changes Nov 20, 2019

View reviewed changes

joshdover approved these changes Nov 20, 2019

View reviewed changes

pgayvallet added 2 commits November 21, 2019 10:50

fix doc and address nits

cfa27e8

Merge remote-tracking branch 'upstream/master' into kbn-41990-client-…

24575b3

…config-values

This comment has been minimized.

Sign in to view

pgayvallet merged commit 1bef133 into elastic:master Nov 21, 2019

kibana-core [DEPRECATED] automation moved this from Code review to Needs Backport Nov 21, 2019

pgayvallet mentioned this pull request Nov 21, 2019

[7.x] Expose whitelisted config values to client-side plugin (#50641) #51272

Merged

pgayvallet moved this from Needs Backport to Done (7.6) in kibana-core [DEPRECATED] Nov 21, 2019

jasonrhodes mentioned this pull request Nov 22, 2019

[APM] New Kibana platform migration plan #32894

Closed

pgayvallet added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels Nov 24, 2019

This was referenced Jan 14, 2020

NP apps with custom appRoute fail to load due to missing legacy injected vars #54470

Closed

[discuss] Introduce render data provider API #54925

Closed

Non-global injected initial data #52553

Open

This was referenced Nov 23, 2021

Remove the legacy types from the platform public contract #77997

Closed

Gracefully handle deprecated core API's #119866

Closed

Expose whitelisted config values to client-side plugin #50641

Expose whitelisted config values to client-side plugin #50641

Conversation

pgayvallet commented Nov 14, 2019 • edited

Summary

Checklist

For maintainers

Dev Docs

elasticmachine commented Nov 14, 2019

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

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Nov 18, 2019 • edited

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

pgayvallet Nov 18, 2019 • edited

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

elasticmachine commented Nov 18, 2019

💚 Build Succeeded

This comment has been minimized.

elasticmachine commented Nov 19, 2019

💔 Build Failed

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

elasticmachine commented Nov 20, 2019

💚 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

This comment has been minimized.

elasticmachine commented Nov 21, 2019

💚 Build Succeeded

eliperelman commented Nov 23, 2019

pgayvallet commented Nov 24, 2019

elasticmachine commented Nov 24, 2019

💚 Build Succeeded

pgayvallet commented Nov 14, 2019 •

edited

mshustov Nov 18, 2019 •

edited

pgayvallet Nov 18, 2019 •

edited