covalent icon indicating copy to clipboard operation
covalent copied to clipboard

Echarts expected on 'window'

Open vhdirk opened this issue 2 years ago • 6 comments

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?

vhdirk avatar May 06 '22 09:05 vhdirk

@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"
 ]

owilliams320 avatar May 11 '22 18:05 owilliams320

@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.

vhdirk avatar May 11 '22 19:05 vhdirk

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

owilliams320 avatar May 11 '22 21:05 owilliams320

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

vhdirk avatar May 12 '22 04:05 vhdirk

@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?

JoshSchoen avatar May 12 '22 14:05 JoshSchoen

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

vhdirk avatar May 12 '22 20:05 vhdirk