Skip to content

chore(NA): load canvas template files on request#62784

Closed
mistic wants to merge 8 commits intoelastic:masterfrom
mistic:reduce-bundles-size
Closed

chore(NA): load canvas template files on request#62784
mistic wants to merge 8 commits intoelastic:masterfrom
mistic:reduce-bundles-size

Conversation

@mistic
Copy link
Contributor

@mistic mistic commented Apr 7, 2020

That is an attempt to load the canvas templates on request instead of bundled them inline. It also prevents the removal of the template proposed here #62688.

I probably miss some good practice around the new platform plugins lifecycle as I usually don't touch that part of the product. Open for discussion 😃

@mistic mistic added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v7.8.0 labels Apr 7, 2020
@mistic mistic requested review from crob611 and spalger April 7, 2020 13:44
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

I don't think we should be downloading the templates as soon as the canvas app is mounted. Why not download the template once it is selected?

I think that we should do something like:

import pitchTemplateUrl from '!!file-loader!./pitch_presentation.json';

const TEMPLATES = [
  {
    name: 'Branded presentation with large photos',
    tags: ['presentation'],
    ...
    url: pitchTemplateUrl
  }
  ...
]

Then, only once a template is chosen from the list UI do we need to fetch() the url in order to initialize the new workpad, and in the vast majority of cases when we're not creating a workpad from the pitch template we don't have to download and parse that JSON.

async mount(context, params) {
// TODO: Do we want to completely move canvas_plugin_src into it's own plugin?
const srcPlugin = new CanvasSrcPlugin();
await srcPlugin.setup(core, { canvas: canvasApi });
Copy link
Contributor

Choose a reason for hiding this comment

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

plugin setup() functions have a specified meaning and they shouldn't be async, if we want to follow the plugin style here I don't think we should block the canvas app mounting by downloading all the templates.

// NOTE: that rule prevents the bundling of json templates on canvas
{
test: /\.json$/,
include: /[\/\\]plugins[\/\\]canvas[\/\\]canvas_plugin_src[\/\\]templates[\/\\].*\.json$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we prefix imports with !!file-loader! instead? Hard coding loaders into the base webpack config for specific plugins is something I'd like to avoid if possible.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mistic mistic removed the v7.7.0 label Apr 11, 2020
@mistic
Copy link
Contributor Author

mistic commented Apr 11, 2020

Good suggestions @spalger. That would imply to change the templates registry. I'll close this for now.

@mistic mistic closed this Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants