search

Teradata / covalent

Teradata UI Platform built on Angular Material
https://teradata.github.io/covalent/
MIT License
2.23k stars 358 forks source link

Echarts expected on 'window' #1933

Open vhdirk opened 2 years ago

vhdirk commented 2 years ago

Describe the bug The function registerThemes, but also pretty much every other class/function, expects the echarts object to be available in the global scope. At first I though this was only for lazily loaded modules, but I can reproduce it for the root component as well.

Stacktrace:

Uncaught ReferenceError: echarts is not defined
    registerTheme covalent-echarts-base.mjs:4321
    registerDefaultThemes covalent-echarts-base.mjs:4328
    7958 covalent-echarts-base.mjs:4819
    Webpack 13
covalent-echarts-base.mjs:4321:4

To Reproduce Steps to reproduce the behavior:

  1. Clone and run this project: https://github.com/vhdirk/covalent-echarts-test
  2. Click the link on the page
  3. See error in console

Expected behavior Echarts should just load. At this point, it is just not usable.

Desktop: any

Smartphone: any

Additional context It should be possible to import the echarts library using import * as echarts from 'echarts'; . However, I think the echarts object should be a singleton, so it should be provided in a service (that is provededIn: 'root'). I think the best option would be to clone what they did in: https://www.npmjs.com/package/ngx-echarts#treeshaking-custom-build

I think each covalent echarts submodule could register what it uses on the global 'echarts' object?

owilliams320 commented 2 years ago

@vhdirk as part of the updates to latest angular and echarts. the recommendation is that you pull in echarts using the script array within your angular.json. This is due to the way echarts is packaged as a CommonJs Module.

example from our app

 "scripts": [
     "node_modules/echarts/dist/echarts.js"
 ]
vhdirk commented 2 years ago

@owilliams320 Thanks for the reply. I realize that it can indeed work that way. However, we're then completely missing out on the tree-shaking capabilities, which would be quite sad.

owilliams320 commented 2 years ago

Yes that would be an issue needed to be raised with echarts project. The problem is their package is only commonjs

vhdirk commented 2 years ago

I'll create an issue with them then. What package type would it need to be?

JoshSchoen commented 2 years ago

@owilliams320 I'm pretty sure v5 introduced modules for better tree shaking. I know I seen it in echart for react and they have an example on their site. @vhdirk any interest in taking a look and contributing?

vhdirk commented 2 years ago

@JoshSchoen I mentioned in the gitter channel that I'd be willing to contribute, yes.