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

series.update() turns es6 classes to objects in userOptions #6046

Closed
yogevyuval opened this Issue Nov 28, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@yogevyuval

yogevyuval commented Nov 28, 2016

Expected behaviour

userOptions should keep their type when they are es6 classes.

Actual behaviour

I tried putting an object in a series userOptions like so:

chart.addSeries({ ..., mySpecialOption: myObject })

where myObject is an instance of class I created like following (es6 syntax):

class SomeClass { constructor() { } ... } myObject = new SomeClass();

So putting the option works fine, but when I call series.update() mySpecialOption becomes a different object of type 'Object'. Then my original instance is gone and i cannot use it as planned. :(

I think it is probably caused by H.merge or something in that area...

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Dec 1, 2016

Collaborator

I think it is probably caused by H.merge or something in that area...

Yes, you're probably right, the object is mutated and stored in the series object itself. A workaround would be to pass a copy of the configuration object to addSeries.

Collaborator

TorsteinHonsi commented Dec 1, 2016

I think it is probably caused by H.merge or something in that area...

Yes, you're probably right, the object is mutated and stored in the series object itself. A workaround would be to pass a copy of the configuration object to addSeries.

@yogevyuval

This comment has been minimized.

Show comment
Hide comment
@yogevyuval

yogevyuval Dec 1, 2016

I did not quite understand your workaround. Can you provide a code sample?

I'm passing the instance of my es6 type to addSeries

yogevyuval commented Dec 1, 2016

I did not quite understand your workaround. Can you provide a code sample?

I'm passing the instance of my es6 type to addSeries

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi
Collaborator

TorsteinHonsi commented Dec 5, 2016

@yogevyuval

This comment has been minimized.

Show comment
Hide comment
@yogevyuval

yogevyuval commented Apr 8, 2017

Any update on this one?
@jon-a-nygaard @TorsteinHonsi

@jon-a-nygaard

This comment has been minimized.

Show comment
Hide comment
@jon-a-nygaard

jon-a-nygaard Apr 18, 2017

Collaborator

@yogevyuval Apologies for the late reply.
There is a possible workaround for you to override Highcharts.isObject, which is used in the merge function, to have it return false when it is a class.

Internal Note:
In stead of modifying isObject, it is probably better to check if the object is a class in the merge function around L449 in Utilities.js

Collaborator

jon-a-nygaard commented Apr 18, 2017

@yogevyuval Apologies for the late reply.
There is a possible workaround for you to override Highcharts.isObject, which is used in the merge function, to have it return false when it is a class.

Internal Note:
In stead of modifying isObject, it is probably better to check if the object is a class in the merge function around L449 in Utilities.js

@yogevyuval

This comment has been minimized.

Show comment
Hide comment
@yogevyuval

yogevyuval Apr 18, 2017

@jon-a-nygaard Thanks for the reply.
While overriding isObject is possible this seems like a very patchy solution :) I prefer to stick with my current patch...

A fix to the H.merge function seems like a good idea, would love to see it :) es6 classes are becoming very popular and I think it's necessary that Highcharts will handle them correctly.

yogevyuval commented Apr 18, 2017

@jon-a-nygaard Thanks for the reply.
While overriding isObject is possible this seems like a very patchy solution :) I prefer to stick with my current patch...

A fix to the H.merge function seems like a good idea, would love to see it :) es6 classes are becoming very popular and I think it's necessary that Highcharts will handle them correctly.

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