ES6 import instead of polluting global namespace

[smnbbrv]

Hi @xieziyu ,

thank you for beautiful project!

I have a question concerning the setup.

README: https://github.com/xieziyu/ngx-echarts#how-to-use-it-within

{
  // projects ...
  "architect": {
    "build": {
      "options": {
        "scripts": [
+         "node_modules/echarts/dist/echarts.min.js"
        ]
      }
    }
  }
}

And all lines like https://github.com/xieziyu/ngx-echarts/blob/4852a0aebed57ee949069338de94b9cc160a5ebb/projects/ngx-echarts/src/lib/ngx-echarts.directive.ts#L9

declare var echarts: any;

This is what I find a little bit dirty. I can't use typings from original echarts to use it with options etc.

Also it creates issues for the people like https://github.com/xieziyu/ngx-echarts/issues/119. They need to redeclare the variables on their own instead of simple

import * as echarts from 'echarts';

I have a working solution with typings, normal import from echarts and fully-functional AOT (if this is the reason of all listed above).

I just want to ask you before making a pull request: is it helpful? Is there any reason why we should go on with dirty importing?

[smnbbrv]

The main use case is actually:

import { EChartOption } from 'echarts';

// ...

export class MyComponent {

  activityChartOptions: EChartOption;

}

[xieziyu]

@smnbbrv Yes, it seems a little bit dirty about the declare. I can't agree more about that! But it's really necessary and important to keep our ngx-echarts lightweight and robust to echarts version.

If we imported echarts in our directive, the echarts' codes would be bundled into our library. It's unnecessary, more like a disaster.

ngx-echarts is designed to be a lite wrapper and it doesn't concern about echarts version too much. By using declare, echarts is becoming a real external library.

That's why you need to install echarts first when you use ngx-echarts. Meanwhile, you can upgrade echarts as you wish without waiting for a new version of ngx-echarts.

[smnbbrv]

@xieziyu sorry, I don't understand...

I don't offer to bundle the specific version of echarts inside of ngx-echarts.

If we imported echarts in our directive, the echarts' codes would be bundled into our library. It's unnecessary, more like a disaster.

Why do you think so? Angular heavily uses rxjs which they internally import literally everywhere but it is not bundled into e.g. @angular/core.

Another example: I am an author of https://github.com/SortableJS/angular-sortablejs. I use original sortablejs library inside as a normal import and also install it separately from the binding library.

The versions are not anyhow restricted (only in package.json peerDependencies).

[smnbbrv]

@xieziyu please check my fork in the related pull request and feel the power of typings :)

then try to build the library and look into the dist folder: there is no original library there.

[xieziyu]

@smnbbrv Thanks very much. You are a great help! When I made the first version of this lib a year ago, I don't have the @types/echarts. At that time, I did try to import echarts functions into the lib. But the echarts codes were bundled into it as I told you. So I had to use declare in turn. Now time is different, I think it's great to use ES6 imports instead.

I will leave comments in your PR.

[smnbbrv]

you are welcome. You should not offer excuses, I know the real pain of supporting the binding lib between two other libs + frontend develops so fast that it is really hard to be up-to-date everywhere. That's why we have open source :)

If you need any further help, feel free to share!

[xieziyu]

@smnbbrv I submit some changes to dev branch. Please help to review the changes if you have time. Thank you!

[smnbbrv]

Looks good to me. Is it already published under v4.0.0-beta.0?

[xieziyu]

Sorry for the late response. I published it just now.

[smnbbrv]

No problem :)

Added a small readme pull request.

I just tested everything, looks super! Thank you very much!

I faced a small issue with the library @angular/flex-layout but I will open another issue because it is unrelated here.