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.

Sign up for GitHub

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

Jump to bottom

Move canvas interpreter to core #24041

Closed
wants to merge 9 commits into from
Closed

Move canvas interpreter to core #24041

wants to merge 9 commits into from

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Oct 15, 2018 •
edited by markov00
Loading

preparation for #23185

moves some parts of canvas to kibana core:

  • interpreter parse functions are moved to @kbn/interpreter
  • function and type registires are moved to @kbn/interpreter
  • all types are moved to @kbn/interpreter
  • clog common function is moved to @kbn/interpreter
  • get_auth, socket, plugin and translate server routes are moved to the interpreter core_plugin
  • browser socket wrapper is moved to the interpreter core_plugin

qa: nothing should change functionally

to test locally:

yarn kbn clean & yarn kbn bootstrap
yarn es snapshot --license trial -E path.data=../data_trial
yarn kbn run build:plugins --include canvas
yarn start

@ppisljar ppisljar force-pushed the move/canvasInterpreterToCore branch 2 times, most recently from 927a868 to 504dfd2 Compare October 15, 2018 17:57
@elastic elastic deleted a comment Oct 15, 2018
@ppisljar ppisljar mentioned this pull request Oct 15, 2018
Closed
@ppisljar ppisljar force-pushed the move/canvasInterpreterToCore branch from 504dfd2 to 5697e42 Compare October 15, 2018 18:38
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the move/canvasInterpreterToCore branch from 77657d2 to b548788 Compare October 16, 2018 08:02
@elastic elastic deleted a comment from elasticmachine Oct 16, 2018
@elastic elastic deleted a comment from elasticmachine Oct 16, 2018
@ppisljar ppisljar requested a review from timroes October 16, 2018 08:05
@ppisljar ppisljar added chore v7.0.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Oct 16, 2018
@ppisljar ppisljar force-pushed the move/canvasInterpreterToCore branch from b548788 to 2cbced3 Compare October 16, 2018 08:30
@elastic elastic deleted a comment from elasticmachine Oct 16, 2018
ppisljar
ppisljar commented Oct 16, 2018
View reviewed changes
packages/kbn-interpreter/common/lib/paths_registry.js Outdated

import clone from 'lodash.clone';

class PathsRegistry {
Copy link
Member Author

Choose a reason for hiding this comment

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

each plugin can register paths to this registry (during init) which we will search for function bundles.

ppisljar
ppisljar commented Oct 16, 2018
View reviewed changes
packages/kbn-interpreter/server/get_plugin_paths.js
export const getPluginPaths = type => {
const typePath = pluginPaths[type];
if (!typePath) throw new Error(`Unknown type: ${type}`);
const typePaths = pathsRegistry.get(type);
Copy link
Member Author

Choose a reason for hiding this comment

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

we get all the paths we need to search from the registry

ppisljar
ppisljar commented Oct 16, 2018
View reviewed changes
src/core_plugins/interpreter/public/plugins_loading_state.js

import uuid from 'uuid/v4';

class PluginsLoadingState {
Copy link
Member Author

Choose a reason for hiding this comment

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

takes care of handling the plugin loading state on the client side. plugins call the loadBrowserPlugins(), which is async function, inside a hack. Everywhere we need loading of browser plugins to be complete, we should use this service to wait for all the browser plugins to be loaded.

ppisljar
ppisljar commented Oct 16, 2018
View reviewed changes
src/core_plugins/interpreter/public/socket.js

pluginsLoaded.then(() => socket.emit('functionList', functionsRegistry.toJS()));
pluginsLoadingState.loadingComplete().then(() => {
socket.emit('functionList', functionsRegistry.toJS());
Copy link
Member Author

Choose a reason for hiding this comment

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

wait for all the browser plugins to be loaded, then emit the functionList

ppisljar
ppisljar commented Oct 16, 2018
View reviewed changes
src/core_plugins/interpreter/server/lib/get_plugin_stream.js

export const getPluginStream = type => {
const stream = ss();
const stream = ss({
separator: '\n',
Copy link
Member Author

Choose a reason for hiding this comment

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

not using the separator sometimes produces broken js files, when joining multiple webpack bundles

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar changed the title [WIP] Move canvas interpreter to core Move canvas interpreter to core Oct 16, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the move/canvasInterpreterToCore branch from 9dcbb16 to 1851fc8 Compare October 16, 2018 15:45
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar
Copy link
Member Author

fail: "apis Monitoring APM overview should summarize apm cluster with metrics"

jenkins, test this

markov00
markov00 reviewed Oct 16, 2018
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

The overall code LGTM.
I've currently only reviewed the code and didn't run any manual test already.
Will run them before closing my review

package.json Outdated
"babel-plugin-inline-react-svg": "^0.5.4",
"babel-plugin-mock-imports": "^0.0.5",
"babel-plugin-pegjs-inline-precompile": "^0.1.0",
"babel-plugin-transform-react-remove-prop-types": "^0.4.14",
"babel-register": "6.18.0",
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason for these plugins to be part of the dependencies? Babel plugins are usually configured as devDependencies

packages/kbn-interpreter/common/lib/arg.js Outdated
import { includes } from 'lodash';

export function Arg(config) {
if (config.name === '_') throw Error('Arg names must not be _. Use it in aliases instead.');
Copy link
Member

Choose a reason for hiding this comment

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

nit: add curly bracket

packages/kbn-interpreter/common/lib/arg.js Outdated
this.multi = config.multi == null ? false : config.multi;
this.resolve = config.resolve == null ? true : config.resolve;
this.accepts = type => {
if (!this.types.length) return true;
Copy link
Member

Choose a reason for hiding this comment

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

nit: add curly bracket

packages/kbn-interpreter/common/lib/paths_registry.js Outdated
@@ -0,0 +1,69 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

As far as I know this is a new file. Can we directly rewrite this in typescript?

packages/kbn-interpreter/common/lib/paths_registry.js Outdated
}

register = (type, paths) => {
if (!type) throw new Error(`Register requires a type`);
Copy link
Member

Choose a reason for hiding this comment

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

nit: add curly brackets

packages/kbn-interpreter/server/load_server_plugins.js Outdated
delete global._;

if (remainingTypes.length) loadType();
else resolve(true);
Copy link
Member

Choose a reason for hiding this comment

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

nit: curly brackets

packages/kbn-interpreter/tasks/webpack.plugins.js Outdated
this.plugin('done', function (stats) {
if (stats.compilation.errors && stats.compilation.errors.length) {
if (isWatch) console.error(stats.compilation.errors[0]);
else throw stats.compilation.errors[0];
Copy link
Member

Choose a reason for hiding this comment

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

nit: curly brackets

src/core_plugins/interpreter/plugin_paths.js

import { resolve } from 'path';

const dir = resolve(__dirname, '..', '..', '..');
Copy link
Member

Choose a reason for hiding this comment

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

👾👾👾

src/dev/license_checker/config.js
@@ -75,4 +75,7 @@ export const LICENSE_OVERRIDES = {
'uglify-js@2.2.5': ['BSD'],
'png-js@0.1.1': ['MIT'],
'sha.js@2.4.11': ['BSD-3-Clause AND MIT'],

// TODO: can be removed once a new version is published
'babel-plugin-mock-imports@0.0.5': ['MIT'],
Copy link
Member

Choose a reason for hiding this comment

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

there is a new version of babel-plugin-mock-imports released 19 days ago :)

x-pack/plugins/canvas/tasks/index.js
import plugins from './plugins';
import prepare from './prepare';
import test from './test';

export default function canvasTasks(gulp, gulpHelpers) {
dev(gulp, gulpHelpers);
peg(gulp, gulpHelpers);
Copy link
Member

Choose a reason for hiding this comment

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

is this entirely substituted by the pegjs common/lib/grammar.peg command on package.json?

Copy link
Member Author

Choose a reason for hiding this comment

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

except for the dev part (where it watches file for changes)

@markov00
Copy link
Member

I've also tested canvas and seems that everything is working as expected

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar
Copy link
Member Author

the same functional test is also failing on master for the last few hours

@elasticmachine
Copy link
Contributor

💔 Build Failed

markov00
markov00 reviewed Oct 17, 2018
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

I've reviewed the changes. I've wrote down some TS comment that I think we need to adress

packages/kbn-interpreter/common/lib/paths_registry.ts
register = (type, paths) => {
if (!type) throw new Error(`Register requires a type`);
public register = (type: string, paths: any) => {
if (!type) {
Copy link
Member

Choose a reason for hiding this comment

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

It's better to restrict it to string or as my previous commit, restrict it to PathLike

packages/kbn-interpreter/common/lib/paths_registry.ts Outdated
}

pathArray.forEach(p => {
this._paths[lowerCaseType].push(p);
// @ts-ignore
this.paths.get(lowerCaseType).push(p);
Copy link
Member

Choose a reason for hiding this comment

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

since you are relatively sure that you have a map object with that name you can also be use the following this.paths.get(lowerCaseType)!.push(p);
You can also rewrite this in a more functional way like:

if (this.paths.has(lowerCaseType)) {
    const currentPathArray = this.paths.get(lowerCaseType);
    this.paths.set(lowerCaseType, [...currentPathArray, ...pathArray]);
} else {
    this.paths.set(lowerCaseType, []);
}

packages/kbn-interpreter/common/lib/paths_registry.ts Outdated

get = (type) => {
if (type === undefined) return [];
public get = (type: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

you should enforce the type output of this function:
public get = (type: string): string[] => {

so that in the future the return type is not dependant on the current implementation but depends on the method type signature: e.g. if in the future you change the implementation and add a code that return null, you will receive a warning/error from ts because of your method signature

packages/kbn-interpreter/common/lib/paths_registry.ts Outdated
get = (type) => {
if (type === undefined) return [];
public get = (type: string) => {
if (type === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

so using typescript this never happen, but in the context of js/ts mix we have it's fine.
Maybe check if type is something different from a string, because someone can also add in a number or a boolean, an object or null.

packages/kbn-interpreter/common/lib/paths_registry.ts
registerAll = (paths) => {
Object.keys(paths).forEach(key => {
this.register(key, paths[key]);
public registerAll = (paths: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

To keep it consistent with the rest of the file, it's better to use string or PathLike as type

Copy link
Member Author

@ppisljar ppisljar Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

here paths is an object with { type: PathLike[], .... }

packages/kbn-interpreter/common/lib/paths_registry.ts
class PathsRegistry {
private paths: Map<string, string[]>;
Copy link
Member

Choose a reason for hiding this comment

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

I've read a bit where this code is executed, this should be a different type signature like
Map<string, PathLike[]> using PathLike from fs lib. the PathLike type should be then used in the rest of this file for a better type checking

@elasticmachine
Copy link
Contributor

💔 Build Failed

lukeelmers
lukeelmers reviewed Oct 19, 2018
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Looks good! Marco already covered a few comments I had, just added a couple more notes. I have not yet tested locally either; will wait to do that until everything is updated with master.

packages/kbn-interpreter/common/lib/get_type.js
export function getType(node) {
if (node == null) return 'null';
if (typeof node === 'object') {
if (!node.type) throw new Error('Objects must have a type propery');
Copy link
Member

Choose a reason for hiding this comment

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

missing t in the word propery

Suggested change
if (!node.type) throw new Error('Objects must have a type propery');
if (!node.type) throw new Error('Objects must have a type property');

packages/kbn-interpreter/common/lib/paths_registry.js Outdated
};

toArray = () => {
return Object.keys(this._paths).map(key => this.get(key));
Copy link
Member

Choose a reason for hiding this comment

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

Would Object.values(this._paths); have the same effect here? Or do we need to be calling this.get()? Perhaps that was the intent due to the cloning...

packages/kbn-interpreter/common/lib/paths_registry.js Outdated
get = (type) => {
if (type === undefined) return [];
const lowerCaseType = type.toLowerCase();
return this._paths[lowerCaseType] ? clone(this._paths[lowerCaseType]) : [];
Copy link
Member

Choose a reason for hiding this comment

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

if it is just a shallow clone, you could also do [...this._paths[lowerCaseType]] instead of using lodash

src/core_plugins/interpreter/public/plugins_loading_state.js

setLoading() {
const id = uuid();
this.states[id] = true;
Copy link
Member

Choose a reason for hiding this comment

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

It might be cleaner to do this.states = new Set(); in the constructor. That way you don't need to arbitrarily set true here and can use add() delete() has() throughout the rest of the class.

packages/kbn-interpreter/server/get_plugin_paths.js
return;
}
return resolve(path, file);
}).filter(path => path)).catch();
Copy link
Member

Choose a reason for hiding this comment

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

what's .filter(path => path) actually doing? and is the empty .catch just there to swallow errors?

@ppisljar ppisljar removed the review label Oct 19, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar
Copy link
Member Author

closed in favor of #25711

@ppisljar ppisljar closed this Nov 15, 2018
w33ble added a commit to w33ble/kibana that referenced this pull request Nov 21, 2018
@w33ble
chore: revert elastic#24041
c3aa48a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukeelmers lukeelmers lukeelmers left review comments

@markov00 markov00 markov00 left review comments

@timroes timroes Awaiting requested review from timroes

@rashidkpc rashidkpc Awaiting requested review from rashidkpc

@lukasolson lukasolson Awaiting requested review from lukasolson

Assignees
No one assigned
Labels
chore Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ppisljar @elasticmachine @markov00 @lukeelmers