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

[NP] Vega migration #63849

Merged
merged 17 commits into from
Apr 24, 2020
Merged

[NP] Vega migration #63849

merged 17 commits into from
Apr 24, 2020

Conversation

maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented Apr 17, 2020

Summary

Part of #60097.

The Vega plugin migration to new platform.

@maryia-lapata maryia-lapata added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.8.0 labels Apr 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293
Copy link
Contributor

@maryia-lapata Could you try whether this fix solves your problem: #63886 ?

Thanks @mistic!

@maryia-lapata
Copy link
Contributor Author

@maryia-lapata Could you try whether this fix solves your problem: #63886 ?

Thanks @mistic!

@flash1293, unfortunately, it didn't help. I've got the same issue:
image

@flash1293
Copy link
Contributor

@maryia-lapata Just tested and it seems to work. As this is in a package, you have to run yarn kbn bootstrap after changing the webpack config for it to take effect.

Then it fails because data is missing in the list of required plugins, but that seems solvable.

@flash1293 flash1293 mentioned this pull request Apr 21, 2020
69 tasks
@maryia-lapata
Copy link
Contributor Author

maryia-lapata commented Apr 21, 2020

I still have a warning, but everything seems to work fine.


np bld    log   [12:04:30.392] [error][@kbn/optimizer] webpack compile errors
   │np bld    log   [12:04:30.416] [error][@kbn/optimizer] [visTypeVega] build
       │np bld    log   [12:04:30.419] [error][@kbn/optimizer] Optimizations failure.
       │         1004 modules
       │
       │          WARNING in /Users/maryia/Documents/elastic/kibana/node_modules/vega-lib/build/vega.js 8473:19-35
       │          Critical dependency: the request of a dependency is an expression
       │              at CommonJsRequireContextDependency.getWarnings (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/dependencies/ContextDependency.js:40:18)
       │              at Compilation.reportDependencyErrorsAndWarnings (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1454:24)
       │              at hooks.finishModules.callAsync.err (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1258:10)
       │              at AsyncSeriesHook.eval [as callAsync] (eval at create (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:15:1)
       │              at AsyncSeriesHook.lazyCompileHook (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/node_modules/tapable/lib/Hook.js:154:20)
       │              at Compilation.finish (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1253:28)
       │              at hooks.make.callAsync.err (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compiler.js:672:17)
       │              at _err0 (eval at create (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:11:1)
       │              at _addModuleChain (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1185:12)
       │              at processModuleDependencies.err (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1097:9)
       │           @ ./public/vega_view/vega_view.js
       │           @ ./public/vega_visualization.js
       │           @ ./public/vega_type.ts
       │           @ ./public/plugin.ts
       │           @ ./public/index.ts
       │
       │          WARNING in /Users/maryia/Documents/elastic/kibana/node_modules/encoding/lib/iconv-loader.js 9:12-34
       │          Critical dependency: the request of a dependency is an expression
       │              at CommonJsRequireContextDependency.getWarnings (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/dependencies/ContextDependency.js:40:18)
       │              at Compilation.reportDependencyErrorsAndWarnings (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1454:24)
       │              at hooks.finishModules.callAsync.err (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1258:10)
       │              at AsyncSeriesHook.eval [as callAsync] (eval at create (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:15:1)
       │              at AsyncSeriesHook.lazyCompileHook (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/node_modules/tapable/lib/Hook.js:154:20)
       │              at Compilation.finish (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1253:28)
       │              at hooks.make.callAsync.err (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compiler.js:672:17)
       │              at _err0 (eval at create (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:11:1)
       │              at _addModuleChain (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1185:12)
       │              at processModuleDependencies.err (/Users/maryia/Documents/elastic/kibana/node_modules/webpack/lib/Compilation.js:1097:9)
       │           @ /Users/maryia/Documents/elastic/kibana/node_modules/encoding/lib/encoding.js
       │           @ /Users/maryia/Documents/elastic/kibana/node_modules/node-fetch/lib/body.js
       │           @ /Users/maryia/Documents/elastic/kibana/node_modules/node-fetch/index.js
       │           @ /Users/maryia/Documents/elastic/kibana/node_modules/vega-lib/build/vega.js
       │           @ ./public/vega_view/vega_view.js
       │           @ ./public/vega_visualization.js
       │           @ ./public/vega_type.ts
       │           @ ./public/plugin.ts
       │           @ ./public/index.ts

But unfortunately this warning fails CI.

@flash1293
Copy link
Contributor

Looking up what this means I'm pretty sure it's caused by this part of the vega code:

  ['canvas', 'canvas-prebuilt'].some(function(libName) {
    try {
      NodeCanvas = require(libName);
      if (typeof NodeCanvas !== 'function') {
        NodeCanvas = null;
      }
    } catch (error) {
      NodeCanvas = null;
    }
    return NodeCanvas;
  });

@mistic Not sure whether it's the right approach but can we ignore the warning in this specific instance? As vega is only running in the client, we don't need the canvas or canvas-prebuilt packages so ignoring this require completely would be fine if that helps.

@maryia-lapata maryia-lapata requested a review from flash1293 April 22, 2020 06:30
@maryia-lapata maryia-lapata marked this pull request as ready for review April 22, 2020 06:30
@maryia-lapata maryia-lapata requested review from a team as code owners April 22, 2020 06:30
@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and works, looks mostly good to me besides for a performance nit and the webpack problem.

src/plugins/vis_type_vega/public/vega_request_handler.ts Outdated Show resolved Hide resolved
@mistic
Copy link
Member

mistic commented Apr 23, 2020

@elasticmachine merge upstream

@mistic
Copy link
Member

mistic commented Apr 23, 2020

It looks like webpack does not consider safe to do that kind of imports on browser code. The current version of vega we are using ends up in a deprecated vega-lib repo. In a first looking at it I didn't spot any good work around on it and it looks like we are not on a position of upgrading to a newer vega version yet. @spalger what do you think about using

stats: {
    warningsFilter: w => ...
  },

and try to just exclude only those 2 warnings for now? Do you have any other idea here?

@flash1293
Copy link
Contributor

It might be too hacky, but for this specific case https://www.npmjs.com/package/webpack-plugin-replace could be an option to just strip out this require statement completely because we don't need it for the plugin to function.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

SASS lgtm

@spalger
Copy link
Contributor

spalger commented Apr 23, 2020

Just pushed a new commit that skips parsing for vega-lib/build/vega.js since the file is prebuilt and webpack doesn't need to search it for require statements at all. Should be good now!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and works as expected, LGTM
Regular vega visualizations as well as maps are still looking fine both in editor and in dashboards

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

changes on the files under operations team code owners LGTM

@maryia-lapata maryia-lapata merged commit 2ace269 into elastic:master Apr 24, 2020
@maryia-lapata maryia-lapata deleted the np-vega branch April 24, 2020 13:25
maryia-lapata added a commit that referenced this pull request Apr 24, 2020
* Vega migartion

* Move mocha tests to legacy

* Fix TS

* Update .i18nrc.json

* Move mocks to vis_type_vega

* Fix issue with babel and vega deps

* Update mocha test

* Mock services

* Update vega_request_handler.ts

* don't parse vega-lib/build/vega.js

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: spalger <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: spalger <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 27, 2020
…bana into ingest-node-pipeline/open-flyout-create-edit

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (116 commits)
  [Ingest Node Pipelines] More lenient treatment of on-failure value (elastic#64411)
  Report Deletion via UI- functional test (elastic#64031)
  Bump handlebars dependency from 4.5.3 to 4.7.6 (elastic#64402)
  [Uptime] Update TLS settings (elastic#64111)
  [alerting] removes usage of any throughout Alerting Services code (elastic#64161)
  [CANVAS] Moves notify to a canvas service (elastic#63268)
  [Canvas] Misc NP Stuff (elastic#63703)
  update apm index pattern (elastic#64232)
  Task/hostlist pagination (elastic#63722)
  [NP] Vega migration (elastic#63849)
  Move ensureDefaultIndexPattern into data plugin (elastic#63100)
  [Fleet] Fix agent status count to not include unenrolled agents (elastic#64106)
  Migrate graph_workspace saved object registration to Kibana platform (elastic#64157)
  Index pattern management UI -> TypeScript and New Platform Ready (edit_index_pattern) (elastic#64184)
  [ML] EuiDataGrid ml/transform components. (elastic#63447)
  [ML] Moving to kibana capabilities (elastic#64057)
  Move input_control_vis into NP (elastic#63333)
  remove reference to local application service in graph (elastic#64288)
  KQL removes leading zero and breaks query (elastic#62748)
  [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants