Skip to content

[canvas/plugins_build] whitelist specific imports#27100

Closed
spalger wants to merge 4 commits intoelastic:masterfrom
spalger:fix/canvas/plugin-build-extra-modules
Closed

[canvas/plugins_build] whitelist specific imports#27100
spalger wants to merge 4 commits intoelastic:masterfrom
spalger:fix/canvas/plugin-build-extra-modules

Conversation

@spalger
Copy link
Copy Markdown
Contributor

@spalger spalger commented Dec 13, 2018

We discovered that a number of modules from outside of x-pack/plugins/canvas/canvas_plugin_src are being imported and added to the canvas_plugin build. Some minor duplication wouldn't be that big of a problem, but as Canvas integrates more closely with Kibana it causes more and more to get included in the canvas_plugin.

The issue that caught my attention was in #27084 where a small unrelated change caused modules like ui/notify to start ending up in the canvas_plugin build.

Doing a little research I found the following files are imported into the canvas_plugin_src directory from outside of it:

./plugins/canvas/common/lib/dataurl
./plugins/canvas/common/lib/fonts
./plugins/canvas/common/lib/get_colors_from_palette
./plugins/canvas/common/lib/get_legend_config
./plugins/canvas/common/lib/handlebars
./plugins/canvas/common/lib/httpurl
./plugins/canvas/common/lib/palettes
./plugins/canvas/common/lib/pivot_object_array
./plugins/canvas/common/lib/resolve_dataurl
./plugins/canvas/common/lib/unquote_string
./plugins/canvas/common/lib/url
./plugins/canvas/public/components/asset_picker
./plugins/canvas/public/components/datatable
./plugins/canvas/public/components/debug
./plugins/canvas/public/components/enhance/stateful_prop
./plugins/canvas/public/components/error
./plugins/canvas/public/components/file_upload
./plugins/canvas/public/components/loading/loading
./plugins/canvas/public/components/palette_picker
./plugins/canvas/public/components/popover
./plugins/canvas/public/components/shape_picker_mini/index
./plugins/canvas/public/lib/arg_helpers
./plugins/canvas/public/lib/get_id
./plugins/canvas/public/lib/legend_options
./plugins/canvas/public/lib/resolved_arg
./plugins/canvas/public/lib/template_from_react_component

There are an additional 18 modules that are imported in from packages. Some of these modules can be problematic, like packages/kbn-interpreter/target/common/lib/types_registry.js, which stores the types registry in its module scope. This approach to storing state only works when we have a single instance of the module in all of the bundles. If we leave this unchecked I imagine it's only a matter of time before we accidentally pull in modules like https://github.com/elastic/kibana/blob/master/packages/kbn-interpreter/src/public/socket.js#L25

I'm not sure exactly how we should solve this, but I hope folks on the canvas team have ideas.

For now I've created a simple webpack plugin that allows us to whitelist certain directories/files that can be in the canvas_plugin bundle. I naively thought that we would be able to whitelist the canvas_plugin_src directory and node_modules, but shortly realized that packages were also going to be necessary and then that canvas_plugin was already reaching out of itself quite often.

Does canvas have a mechanism for sharing code with plugins that doesn't require importing it from a module? If we went down the path of only whitelisting the node_modules and canvas_plugin_src directory could we pass in all the other dependencies via this mechanism?

@spalger spalger added discuss Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// labels Dec 13, 2018
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-canvas

@spalger spalger force-pushed the fix/canvas/plugin-build-extra-modules branch from b8be11d to 952449a Compare December 13, 2018 01:25
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@rashidkpc
Copy link
Copy Markdown
Contributor

I really like this approach to defining what @spalger calls a "DMZ" for plugin components. This makes our plugin API clean and isolated and an encourages us to write well isolated plugin code.

@w33ble
Copy link
Copy Markdown
Contributor

w33ble commented Dec 13, 2018

A lot of the issues we have here is additional fallout from #26068 😿

@spalger spalger requested a review from a team as a code owner December 13, 2018 18:13
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@w33ble w33ble self-assigned this Dec 13, 2018
@spalger spalger closed this Feb 28, 2019
@spalger spalger deleted the fix/canvas/plugin-build-extra-modules branch August 18, 2020 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discuss Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t//

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants