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

Highchart more cannot be called multiple time without issues #7729

Closed
JSteunou opened this Issue Jan 25, 2018 · 14 comments

Comments

Projects
None yet
6 participants
@JSteunou

JSteunou commented Jan 25, 2018

Expected behaviour

highchartsMore(Highcharts) should be called multiple time without adding issues.

Actual behaviour

Calling highchartsMore multiple time make Highcharts fuzzy, like showing multiple time a spiderweb diagram with the same data each time produce random like result.

With a flag to prevent highchartsMore to be called multiple time, previous case goes well.

Live demo with steps to reproduce

Sorry I dont know how to fiddle it, this is a bug only when using the npm version and asynchronous import like

Promise.all([
    import(/* webpackChunkName: "highcharts" */ 'highcharts'),
    import(/* webpackChunkName: "highcharts" */ 'highcharts/highcharts-more'),
]).then(([Highcharts, highchartsMore]) => {
    if (!ALREADY_EXTENDED_WITH_MORE) {
        highchartsMore(Highcharts);
        ALREADY_EXTENDED_WITH_MORE = true;
    }
    return Highcharts;
});

Product version

6.0.4

Affected browser(s)

All

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Jan 26, 2018

Contributor

Hi @JSteunou

This doesn't look like a bug.

Why would you load the same file multiple times?
What's the expected behavior based on?

The highchartsMore(Highcharts) initiates the module and should be called once for Highcharts instance.

Contributor

KacperMadej commented Jan 26, 2018

Hi @JSteunou

This doesn't look like a bug.

Why would you load the same file multiple times?
What's the expected behavior based on?

The highchartsMore(Highcharts) initiates the module and should be called once for Highcharts instance.

@JSteunou

This comment has been minimized.

Show comment
Hide comment
@JSteunou

JSteunou Jan 27, 2018

Webpack take care of the async load with import() by fetching and returning the chunk at 1st call then always return the same module for consecutive calls. I think this is the same behavior in native browser vendors implementation.

The advantage of this in a big app is having a single point quite like a factory to load Highcharts as external chunk from anywhere in your code, at any time, without worring about is this the 1st load or not.

Having it as external resource is a part of the code splitting logic from webpack and other bundlers. Your app is lighter and your vendor resources can be cached by the browser. And the major point is that the module is only loaded when needed, when imported.

This is why, and the is the modern JS app actual world, my app do not know if Highcharts was already loaded or not (and also because separation of concern) and just ask for it. The factory ask webpack for it and return it. That is it. That should be it.

If everyone has to care and check if the core was already extended by the more then it might be inside the lib or documented to avoid code duplication or error.

JSteunou commented Jan 27, 2018

Webpack take care of the async load with import() by fetching and returning the chunk at 1st call then always return the same module for consecutive calls. I think this is the same behavior in native browser vendors implementation.

The advantage of this in a big app is having a single point quite like a factory to load Highcharts as external chunk from anywhere in your code, at any time, without worring about is this the 1st load or not.

Having it as external resource is a part of the code splitting logic from webpack and other bundlers. Your app is lighter and your vendor resources can be cached by the browser. And the major point is that the module is only loaded when needed, when imported.

This is why, and the is the modern JS app actual world, my app do not know if Highcharts was already loaded or not (and also because separation of concern) and just ask for it. The factory ask webpack for it and return it. That is it. That should be it.

If everyone has to care and check if the core was already extended by the more then it might be inside the lib or documented to avoid code duplication or error.

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Jan 29, 2018

Contributor

You could create a custom Highcharts file with loaded all modules and then import should handle the check to avoid multiple instances. Here's more about custom Highcharts files: https://www.highcharts.com/docs/getting-started/how-to-create-custom-highcharts-files

App should care about not loading files if they are loaded already, so this should resolved the problem.

If you would like to propose an enhancement idea for a checker that will do nothing on an attempt of loading an already loaded module please open an idea on userVoice of vote for the already registered one if it already exists. The most popular ideas are getting implemented.

Contributor

KacperMadej commented Jan 29, 2018

You could create a custom Highcharts file with loaded all modules and then import should handle the check to avoid multiple instances. Here's more about custom Highcharts files: https://www.highcharts.com/docs/getting-started/how-to-create-custom-highcharts-files

App should care about not loading files if they are loaded already, so this should resolved the problem.

If you would like to propose an enhancement idea for a checker that will do nothing on an attempt of loading an already loaded module please open an idea on userVoice of vote for the already registered one if it already exists. The most popular ideas are getting implemented.

@JSteunou

This comment has been minimized.

Show comment
Hide comment
@JSteunou

JSteunou Feb 9, 2018

Your link about userVoice is broken.

And again, the issue is not loading twice, the modules are loaded only once, but the call to augment HighCharts with HighChartsMore can be done multiple time because the app is stateless and does not know if the call was already made or not.

To avoid all users to write the ugly check I did have to wrote, the check should be moved inside HighChartMore itself.

JSteunou commented Feb 9, 2018

Your link about userVoice is broken.

And again, the issue is not loading twice, the modules are loaded only once, but the call to augment HighCharts with HighChartsMore can be done multiple time because the app is stateless and does not know if the call was already made or not.

To avoid all users to write the ugly check I did have to wrote, the check should be moved inside HighChartMore itself.

@sebastianbochan

This comment has been minimized.

Show comment
Hide comment
@sebastianbochan

sebastianbochan Feb 9, 2018

Contributor

Hi @JSteunou
Fixed link to the userVoice plaform:

Contributor

sebastianbochan commented Feb 9, 2018

Hi @JSteunou
Fixed link to the userVoice plaform:

@TorsteinHonsi TorsteinHonsi added Bug and removed Not a bug labels Feb 12, 2018

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Feb 12, 2018

Collaborator

@JSteunou I agree with you it's a bug.

Rather than looking for whether highcharts-more.js has been previously included, I think the more robust approach would be to do this module by module. In the commit I added two such tests. Adding the test in Polar.js took care of series being painted twice. See the fix applied here: http://jsfiddle.net/txjet2z9/

Are you aware of any issues that come from this, so we should add the tests to other module files to?

Collaborator

TorsteinHonsi commented Feb 12, 2018

@JSteunou I agree with you it's a bug.

Rather than looking for whether highcharts-more.js has been previously included, I think the more robust approach would be to do this module by module. In the commit I added two such tests. Adding the test in Polar.js took care of series being painted twice. See the fix applied here: http://jsfiddle.net/txjet2z9/

Are you aware of any issues that come from this, so we should add the tests to other module files to?

@JSteunou

This comment has been minimized.

Show comment
Hide comment
@JSteunou

JSteunou Feb 12, 2018

I actually discover the issue because of polar charts :) We did not had issue with line charts.

JSteunou commented Feb 12, 2018

I actually discover the issue because of polar charts :) We did not had issue with line charts.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Feb 12, 2018

Collaborator

Lines can be polar too... So did this fix all your known issues?

Collaborator

TorsteinHonsi commented Feb 12, 2018

Lines can be polar too... So did this fix all your known issues?

@JSteunou

This comment has been minimized.

Show comment
Hide comment
@JSteunou

JSteunou Feb 12, 2018

Dont know yet, have to find some bandwidth this week to test it

JSteunou commented Feb 12, 2018

Dont know yet, have to find some bandwidth this week to test it

@dominikkaegi

This comment has been minimized.

Show comment
Hide comment
@dominikkaegi

dominikkaegi Mar 27, 2018

@TorsteinHonsi there is also an issue if you load the drilldown module multiple times. The chart still functions as expected but it throws an error if you want to drill down into a series. See replication here:
http://jsfiddle.net/doka__/440tenn3/

Uncaught TypeError: a.doDrilldown is not a function
    at B.<anonymous> (VM327 drilldown.js:25)
    at VM326 highcharts.js:30
    at Array.forEach (<anonymous>)
    at Object.a.each (VM326 highcharts.js:27)
    at a.fireEvent (VM326 highcharts.js:30)
    at B.firePointEvent (VM326 highcharts.js:268)
    at a.Pointer.onContainerClick (VM326 highcharts.js:205)
    at HTMLDivElement.c.onclick (VM326 highcharts.js:206)

dominikkaegi commented Mar 27, 2018

@TorsteinHonsi there is also an issue if you load the drilldown module multiple times. The chart still functions as expected but it throws an error if you want to drill down into a series. See replication here:
http://jsfiddle.net/doka__/440tenn3/

Uncaught TypeError: a.doDrilldown is not a function
    at B.<anonymous> (VM327 drilldown.js:25)
    at VM326 highcharts.js:30
    at Array.forEach (<anonymous>)
    at Object.a.each (VM326 highcharts.js:27)
    at a.fireEvent (VM326 highcharts.js:30)
    at B.firePointEvent (VM326 highcharts.js:268)
    at a.Pointer.onContainerClick (VM326 highcharts.js:205)
    at HTMLDivElement.c.onclick (VM326 highcharts.js:206)
@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Mar 27, 2018

Contributor

Hi @dominikkaegi - this is reported as separate issue, take a look: #6086.

Contributor

pawelfus commented Mar 27, 2018

Hi @dominikkaegi - this is reported as separate issue, take a look: #6086.

@dominikkaegi

This comment has been minimized.

Show comment
Hide comment
@dominikkaegi

dominikkaegi Mar 28, 2018

@pawelfus I'm unsure if I understood this issue thread correctly. I have seen that work around but as of my understanding in the issue #6086 of 2016 it was decided that Highcharts should not check if you extend your import of Highcharts multiple times with modules e.g. drilldown(Highcharts) or drilldown(Highcharts).

As of my understanding in this recent thread it is seen as a bug and that you want to prevent the possibility of extending Highcharts multiple times and with those errors which occur. Did I misunderstand anything?

dominikkaegi commented Mar 28, 2018

@pawelfus I'm unsure if I understood this issue thread correctly. I have seen that work around but as of my understanding in the issue #6086 of 2016 it was decided that Highcharts should not check if you extend your import of Highcharts multiple times with modules e.g. drilldown(Highcharts) or drilldown(Highcharts).

As of my understanding in this recent thread it is seen as a bug and that you want to prevent the possibility of extending Highcharts multiple times and with those errors which occur. Did I misunderstand anything?

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Mar 28, 2018

Contributor

@dominikkaegi Issue #6086 was labeled as Undecided and Not a bug, which pretty much described the issue. However issues with loading multiple times the same module is now (as of 2018) a common thing, so we want to protect modules from failing anyway.

Contributor

pawelfus commented Mar 28, 2018

@dominikkaegi Issue #6086 was labeled as Undecided and Not a bug, which pretty much described the issue. However issues with loading multiple times the same module is now (as of 2018) a common thing, so we want to protect modules from failing anyway.

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