[interpreter/loadPlugins] avoid deleting globals added by plugins #27171

spalger · 2018-12-13T22:12:37Z

Plugins loaded by the @kbn/interpreter can sometimes setup global values while loading, like the regeneratorRuntime for instance, but the current plugin loading code is deleting every global that was added during plugin load. This changes the logic to only cleanup the canvas global after loading the canvas plugins.

resolves #27162

packages/kbn-interpreter/src/server/require_with_globals.js

elasticmachine · 2018-12-15T01:11:33Z

Pinging @elastic/kibana-canvas

spalger · 2018-12-17T16:54:05Z

I decided I should write tests for the requireWithGlobals() function and it turns out it's not working the way I had intended. Will take a look today and implementing it correctly and including tests to verify that.

rashidkpc

I'm good with this conceptually, we just need to make sure nothing break.

spalger · 2018-12-19T23:58:01Z

src/setup_node_env/babel_register/register.js

-  // `a` is `functions` and `b` is `server`
-  /\/canvas\/canvas_plugin\/(?!functions\/server)([^\/]+\/[^\/]+)/,
+  // ignore paths matching `/canvas/canvas_plugin/`
+  /\/canvas\/canvas_plugin\//,


By sharing the server_code_transformer with @kbn/interpreter I was able to pre-transpile the server code in the canvas_plugin_src and remove this customization.

…generator-runtime-global

Thanks for the review, but we've changed the approach pretty dramatically due to issues/complexity caused by mocking out the module system

elasticmachine · 2018-12-21T00:12:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

clintandrewhall · 2018-12-26T15:50:15Z

packages/kbn-interpreter/src/server/server_registries.js

-          delete global[key];
-        }
-      });
+      delete global.canvas;


Can we add some documentation to this logic, to make it clear why we're adding, registering and then deleting the global?

Are there any other globals we should consider deleting? Forgive the terminology, but perhaps a registry of globals to delete would help, even if it's a bit derivative...?

We add the global for plugins to use. They all use canvas.register, so the global needs to exist when the code is run (ie. when require(path) is run).

My understanding is that it's removed to avoid complaints from our test runners, which check for added globals in the node runtime and fail if any are unexpectedly added.

w33ble

Couple of questions, nothing worth blocking the PR over though.

w33ble · 2018-12-26T21:50:42Z

x-pack/plugins/canvas/tasks/helpers/webpack.plugins.js

@@ -6,6 +6,9 @@

 const path = require('path');
 const CopyWebpackPlugin = require('copy-webpack-plugin');
+const {
+  createServerCodeTransformer,
+} = require('@kbn/interpreter/tasks/build/server_code_transformer');


The interpreter seems like a weird place to import build configuration from. It's also weird that it's importing something that wouldn't be part of the published package.

Do you think putting this in another package (currently existing or not) would make more sense?

We really don't want to have hundreds of packages in two years, so no to another package. I think this is fine, we don't publish these packages and if we want to we can figure something out then.

w33ble · 2018-12-26T21:53:18Z

src/dev/mocha/run_mocha_cli.js

@@ -42,6 +42,8 @@ export function runMochaCli() {

  // check that we aren't leaking any globals
  process.argv.push('--check-leaks');
+  // prevent globals injected from canvas plugins from triggering leak check
+  process.argv.push('--globals', 'core,regeneratorRuntime,_');


Kind of fragile, but at least future globals will break the CI.

I do wonder, is there a way to remove that lodash global?

w33ble · 2018-12-26T22:11:55Z

Confirmed fix for #27162 in dev mode.

Currently creating a build to check #27162 and #27729 in production.

w33ble · 2018-12-26T22:55:16Z

Confirmed this does fix #27162 in the build as well.

Unfortunately, this does not fix #27729.

w33ble

Whoops, thought I approved this already. LGTM.

…astic#27171) Plugins loaded by the `@kbn/interpreter` can sometimes setup global values while loading, like the regeneratorRuntime for instance, but the current plugin loading code is deleting every global that was added during plugin load. This changes the logic to only cleanup the `canvas` global after loading the canvas plugins. resolves elastic#27162

…7171) (#27852) Plugins loaded by the `@kbn/interpreter` can sometimes setup global values while loading, like the regeneratorRuntime for instance, but the current plugin loading code is deleting every global that was added during plugin load. This changes the logic to only cleanup the `canvas` global after loading the canvas plugins. resolves #27162

…7171) (#27851) Plugins loaded by the `@kbn/interpreter` can sometimes setup global values while loading, like the regeneratorRuntime for instance, but the current plugin loading code is deleting every global that was added during plugin load. This changes the logic to only cleanup the `canvas` global after loading the canvas plugins. resolves #27162

w33ble · 2018-12-28T22:24:14Z

6.6 ffe58f4
6.x f07987e

…astic#27171) Plugins loaded by the `@kbn/interpreter` can sometimes setup global values while loading, like the regeneratorRuntime for instance, but the current plugin loading code is deleting every global that was added during plugin load. This changes the logic to only cleanup the `canvas` global after loading the canvas plugins. resolves elastic#27162

spalger force-pushed the fix/canvas-regenerator-runtime-global branch from bb9022b to c071bb9 Compare December 13, 2018 22:27

This comment has been minimized.

Sign in to view

spalger requested a review from a team as a code owner December 14, 2018 20:42

spalger commented Dec 14, 2018

View reviewed changes

packages/kbn-interpreter/src/server/require_with_globals.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

spalger requested review from w33ble and clintandrewhall December 15, 2018 01:11

spalger added review v7.0.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.6.0 labels Dec 15, 2018

ppisljar mentioned this pull request Dec 17, 2018

removing cleaning vars from global #27290

Closed

7 tasks

spalger removed request for clintandrewhall and w33ble December 17, 2018 16:52

spalger added WIP Work in progress and removed review labels Dec 17, 2018

spalger force-pushed the fix/canvas-regenerator-runtime-global branch from eba776d to a34b276 Compare December 17, 2018 17:46

rashidkpc previously approved these changes Dec 17, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

spalger commented Dec 19, 2018

View reviewed changes

[kbn/interpreter] cleanup imports

681f322

This comment has been minimized.

Sign in to view

Merge branch 'master' of github.com:elastic/kibana into fix/canvas-re…

e497403

…generator-runtime-global

This comment has been minimized.

Sign in to view

[mocha] allow a few globals to get tests passing, see elastic#27640

5853717

spalger force-pushed the fix/canvas-regenerator-runtime-global branch from e58dc9a to 5853717 Compare December 20, 2018 23:32

spalger added review and removed WIP Work in progress labels Dec 21, 2018

spalger requested review from w33ble and clintandrewhall December 21, 2018 23:40

clintandrewhall approved these changes Dec 26, 2018

View reviewed changes

w33ble mentioned this pull request Dec 26, 2018

Adding point series elements on Canvas workpads is failing with error [pointseries] > (0 , _lodash2.default) is not a function #27729

Closed

w33ble reviewed Dec 26, 2018

View reviewed changes

w33ble approved these changes Dec 28, 2018

View reviewed changes

w33ble merged commit 3d96469 into elastic:master Dec 28, 2018

w33ble added the v6.7.0 label Dec 28, 2018

w33ble mentioned this pull request Dec 28, 2018

[6.x] [interpreter/loadPlugins] avoid deleting globals added by plugins (#27171) #27851

Merged

w33ble mentioned this pull request Dec 28, 2018

[6.6] [interpreter/loadPlugins] avoid deleting globals added by plugins (#27171) #27852

Merged

spalger deleted the fix/canvas-regenerator-runtime-global branch August 18, 2020 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[interpreter/loadPlugins] avoid deleting globals added by plugins #27171

[interpreter/loadPlugins] avoid deleting globals added by plugins #27171

spalger commented Dec 13, 2018 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Dec 15, 2018

spalger commented Dec 17, 2018

rashidkpc left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

spalger Dec 19, 2018

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Dec 21, 2018

clintandrewhall Dec 26, 2018

w33ble Dec 26, 2018

w33ble left a comment

w33ble Dec 26, 2018

spalger Dec 27, 2018

w33ble Dec 26, 2018

w33ble commented Dec 26, 2018

w33ble commented Dec 26, 2018

w33ble left a comment

w33ble commented Dec 28, 2018 •

edited

Loading

[interpreter/loadPlugins] avoid deleting globals added by plugins #27171

[interpreter/loadPlugins] avoid deleting globals added by plugins #27171

Conversation

spalger commented Dec 13, 2018 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Dec 15, 2018

spalger commented Dec 17, 2018

rashidkpc left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

spalger Dec 19, 2018

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Dec 21, 2018

💚 Build Succeeded

clintandrewhall Dec 26, 2018

Choose a reason for hiding this comment

w33ble Dec 26, 2018

Choose a reason for hiding this comment

w33ble left a comment

Choose a reason for hiding this comment

w33ble Dec 26, 2018

Choose a reason for hiding this comment

spalger Dec 27, 2018

Choose a reason for hiding this comment

w33ble Dec 26, 2018

Choose a reason for hiding this comment

w33ble commented Dec 26, 2018

w33ble commented Dec 26, 2018

w33ble left a comment

Choose a reason for hiding this comment

w33ble commented Dec 28, 2018 • edited Loading

spalger commented Dec 13, 2018 •

edited

Loading

w33ble commented Dec 28, 2018 •

edited

Loading