Skip to content
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

Merged
merged 8 commits into from
Apr 6, 2018
4 changes: 1 addition & 3 deletions src/core_plugins/kibana/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ export default function (kibana) {
'docViews',
'embeddableFactories',
],
injectVars,
},

links: [
Expand Down Expand Up @@ -155,8 +154,7 @@ export default function (kibana) {
registerTutorials(server);
server.expose('systemApi', systemApi);
server.expose('handleEsError', handleEsError);
server.expose('injectVars', injectVars);

server.injectUiAppVars('kibana', () => injectVars(server));
}
});
}
8 changes: 0 additions & 8 deletions src/core_plugins/timelion/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,6 @@ export default function (kibana) {
description: 'Time series expressions for everything',
icon: 'plugins/timelion/icon.svg',
main: 'plugins/timelion/app',
injectVars: function (server) {
const config = server.config();
return {
kbnIndex: config.get('kibana.index'),
esShardTimeout: config.get('elasticsearch.shardTimeout'),
esApiVersion: config.get('elasticsearch.apiVersion')
};
},
uses: [
'fieldFormats',
'savedObjectTypes'
Expand Down
9 changes: 8 additions & 1 deletion src/core_plugins/timelion/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,12 @@ export default function (server) {
getFunction: getFunction
};


server.injectUiAppVars('timelion', () => {
const config = server.config();
return {
kbnIndex: config.get('kibana.index'),
esShardTimeout: config.get('elasticsearch.shardTimeout'),
esApiVersion: config.get('elasticsearch.apiVersion')
};
});
}
10 changes: 5 additions & 5 deletions src/ui/__tests__/fixtures/test_app/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,17 @@ export default kibana => new kibana.Plugin({
app: {
name: 'test_app',
main: 'plugins/test_app/index.js',
injectVars() {
return {
from_test_app: true
};
}
},

injectDefaultVars() {
return {
from_defaults: true
};
}
},
init(server) {
server.injectUiAppVars('test_app', () => ({
from_test_app: true
}));
}
});
93 changes: 93 additions & 0 deletions src/ui/ui_apps/__snapshots__/ui_apps_mixin.test.js.snap
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 {}`;
54 changes: 0 additions & 54 deletions src/ui/ui_apps/__tests__/ui_app.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@ function createStubUiAppSpec(extraParams) {
'chromeNavControls',
'hacks',
],
injectVars() {
return { foo: 'bar' };
},
...extraParams
};
}
Expand Down Expand Up @@ -84,10 +81,6 @@ describe('ui apps / UiApp', () => {
expect(app.getNavLink()).to.be.a(UiNavLink);
});

it('has no injected vars', () => {
expect(app.getInjectedVars()).to.be(undefined);
});

it('has an empty modules list', () => {
expect(app.getModules()).to.eql([]);
});
Expand Down Expand Up @@ -133,10 +126,6 @@ describe('ui apps / UiApp', () => {
expect(app.getNavLink()).to.be(undefined);
});

it('has injected vars', () => {
expect(app.getInjectedVars()).to.eql({ foo: 'bar' });
});

it('includes main and hack modules', () => {
expect(app.getModules()).to.eql([
'main.js',
Expand Down Expand Up @@ -295,49 +284,6 @@ describe('ui apps / UiApp', () => {
});

describe('pluginId', () => {
describe('not specified', () => {
it('passes the root server and undefined for plugin/optoins to injectVars()', () => {
const injectVars = sinon.stub();
const kbnServer = createStubKbnServer();
createUiApp(createStubUiAppSpec({ injectVars }), kbnServer).getInjectedVars();

sinon.assert.calledOnce(injectVars);
sinon.assert.calledOn(injectVars, sinon.match.same(undefined));
sinon.assert.calledWithExactly(
injectVars,
// server arg, uses root server because there is no plugin
sinon.match.same(kbnServer.server),
// options is undefined because there is no plugin
sinon.match.same(undefined)
);
});
});
describe('matches a kbnServer plugin', () => {
it('passes the plugin/server/options from the plugin to injectVars()', () => {
const server = {};
const options = {};
const plugin = {
id: 'test plugin id',
getServer() {
return server;
},
getOptions() {
return options;
}
};

const kbnServer = createStubKbnServer();
kbnServer.plugins.push(plugin);

const injectVars = sinon.stub();
const spec = createStubUiAppSpec({ pluginId: plugin.id, injectVars });
createUiApp(spec, kbnServer).getInjectedVars();

sinon.assert.calledOnce(injectVars);
sinon.assert.calledOn(injectVars, sinon.match.same(plugin));
sinon.assert.calledWithExactly(injectVars, sinon.match.same(server), sinon.match.same(options));
});
});
describe('does not match a kbnServer plugin', () => {
it('throws an error at instantiation', () => {
expect(() => {
Expand Down
25 changes: 4 additions & 21 deletions src/ui/ui_apps/ui_app.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ export class UiApp {
hidden,
linkToLastSubUrl,
listed,
injectVars,
url = `/app/${id}`,
uses = []
} = spec;
Expand All @@ -22,6 +21,10 @@ export class UiApp {
throw new Error('Every app must specify an id');
}

if (spec.injectVars) {
Copy link
Member

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 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?

throw new Error(`uiApp.injectVars has been removed. Use server.injectUiAppVars('${id}', () => { ... })`);
}

this._id = id;
this._main = main;
this._title = title;
Expand All @@ -32,7 +35,6 @@ export class UiApp {
this._hidden = hidden;
this._listed = listed;
this._url = url;
this._injectedVarsProvider = injectVars;
this._pluginId = pluginId;
this._kbnServer = kbnServer;

Expand Down Expand Up @@ -94,25 +96,6 @@ export class UiApp {
}
}

getInjectedVars() {
const provider = this._injectedVarsProvider;
const plugin = this._getPlugin();

if (!provider) {
return;
}

return provider.call(
plugin,
plugin
? plugin.getServer()
: this._kbnServer.server,
plugin
? plugin.getOptions()
: undefined
);
}

getModules() {
return this._modules;
}
Expand Down
14 changes: 14 additions & 0 deletions src/ui/ui_apps/ui_apps_mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];
Copy link
Member

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor Author

@spalger spalger Apr 5, 2018

Choose a reason for hiding this comment

The 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 * 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.

server.decorate('server', 'injectUiAppVars', function (id, provider) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: arrow function maybe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

injectedVarProviders.push({ id, provider, });
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
}), {});
});
}
Loading