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

Update highcharts-chart.component.ts #73

Closed
wants to merge 2 commits into from

Conversation

fclmman
Copy link

@fclmman fclmman commented Sep 24, 2018

Added _zone.runOutsideAngular for highcharts stuff, as it hardly affects application performance due to multiple eventListeners inside highcharts itself

@KacperMadej
Copy link
Contributor

Hi @fclmman

Before we could accept the PR we'll need more info about the issue it's related to and what exactly is the "stuff" you mentioned.

@fclmman
Copy link
Author

fclmman commented Sep 24, 2018

Hi, @KacperMadej, thank for such quick response. What i mentioned is an issue, that can be seen when the application is quite complex, and your page consists of multiple components. It can happen on some enterprise application, where you have huge datagrid and highcharts chart on one page, for example. What i meant by stuff is multiple asynchronous work and eventlisteners that exist inside the highcharts. And as we know it triggers andular's changedetection. So when you track a series in chart with your mouse, highcharts fires lots of point mouseenter events, each of them triggers changedetection, which is really slowing down a huge app. To prevent it we can run highcharts code outside of zone, and prevent multiple changedetections.

@fclmman fclmman closed this Sep 24, 2018
@fclmman fclmman reopened this Sep 24, 2018
@KacperMadej
Copy link
Contributor

@fclmman

Thank you for all the information.
Is the window.setTimeout really needed? Looks like a not a perfect solution.

@fclmman
Copy link
Author

fclmman commented Sep 26, 2018

@KacperMadej well, you were right, no need in timeout here, updated the pull request

@KacperMadej
Copy link
Contributor

I had more time run more tests. The idea is interesting and solution for the problem works. It's possible to still use _zone.run() in chart's events if anyone wants to trigger change detection, so we are not taking away any features. The new way of running the wrapper should be optional to prevent breaking backward compatibility. Because of this the code will look differently, but will be based on your suggestions @fclmman , so thank you very much for all the help.

We'll need to prepare some documentation for the new feature.
I'll shortly open an issue for better tracking.

@fclmman
Copy link
Author

fclmman commented Sep 27, 2018

Well, if you dont mind i can provide optional runotside, so that current behaviour will stay as default

@KacperMadej
Copy link
Contributor

@fclmman

If you could test new version 2.2.0 and check related commit (2ebcb07) to ensure that the suggested solution resolved the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants