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

Axis options can not be cleared with undefined #10525

Closed
bre1470 opened this issue Apr 10, 2019 · 12 comments · Fixed by #16721
Closed

Axis options can not be cleared with undefined #10525

bre1470 opened this issue Apr 10, 2019 · 12 comments · Fixed by #16721

Comments

@bre1470
Copy link
Member

bre1470 commented Apr 10, 2019

If I set tickPositions to null, it magically works, but TS doesn't like it, null is not allowed.
Setting single tickPositions to undefined is working with Highcharts 9.3.1.

See: highcharts/highcharts-react#110

Demo: https://jsfiddle.net/43w5ch2L/

Workaround: https://jsfiddle.net/bre1470/tr6shfy4/
Setting tickPositions options to [] is working with rendering of Highcharts 9.3.1, though the previous options are still accessible via Axis.options.

@bre1470 bre1470 added Type: Undecided Product: TypeScript Issues & enhancements related to TS/TypeScript labels Apr 10, 2019
@bre1470 bre1470 self-assigned this Apr 10, 2019
@bre1470 bre1470 added Type: Bug and removed Type: Undecided Product: TypeScript Issues & enhancements related to TS/TypeScript labels Apr 10, 2019
@bre1470 bre1470 changed the title yAxis tick position can not be cleared with declarations yAxis tick position can not be cleared with undefined Apr 10, 2019
@bre1470 bre1470 removed their assignment Apr 10, 2019
@bre1470 bre1470 changed the title yAxis tick position can not be cleared with undefined Axis options can not be cleared with undefined Apr 10, 2019
@bre1470
Copy link
Member Author

bre1470 commented Apr 10, 2019

The same problem exists with max and min options.

@TorsteinHonsi
Copy link
Collaborator

but TS doesn't like it, null is not allowed

Should we allow null in the type definitions rather than change the behaviour?

@bre1470
Copy link
Member Author

bre1470 commented Apr 15, 2019

I agree in the sense, that we should continue to support null for backward compatibility, but officially we should only advertise undefined in documentation and declarations.

Otherwise it would get inconsistent with the behaviour in all other parts. It is also easier to handle undefined than null. For example you do not end up with functions that have optional parameter with a type of Array<number>|null|undefined, where the last two have the exact same result.

@bre1470
Copy link
Member Author

bre1470 commented Nov 4, 2019

Probably a related issue: #10525

@stale
Copy link

stale bot commented Jun 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

@stale stale bot added the Status: Stale This issue hasn't had any activity for a while, and will be auto-closed if no further updates occur. label Jun 3, 2021
@stale stale bot closed this as completed Jun 11, 2021
@jamesonhill
Copy link

Hi,

Can this issue be re-opened? It's still a problem on 9.2.2

@bre1470
Copy link
Member Author

bre1470 commented Nov 11, 2021

Hi @jamesonhill,
Thank you for the feedback. It is working for some options.
Workaround in this particular case is to set tickPositions to an empty array.
Though the tickPositions option is still set on the axis, the renderer does not add the ticks anymore.

Modified demo: https://jsfiddle.net/bre1470/tr6shfy4/

@bre1470 bre1470 reopened this Nov 11, 2021
@stale stale bot removed the Status: Stale This issue hasn't had any activity for a while, and will be auto-closed if no further updates occur. label Nov 11, 2021
@jamesonhill
Copy link

@bre1470 Setting it to empty array causes no tick marks to display. In my case, I want to let Highcharts decide how the ticks are plotted. Currently it works if the initial value is undefined, but if you change it from non-undefined -> undefined the ticks don't update. Setting it to null and casting the variable to any is needed to satisfy TS, which isn't great but works.

@bre1470
Copy link
Member Author

bre1470 commented Nov 11, 2021

@jamesonhill Thank you for the details about your use case.

@TorsteinHonsi The use case reads like we have to support null as the "automatic" value instead of a tick array. What do you think?

@TorsteinHonsi
Copy link
Collaborator

@bre1470 Actually I think this is a bug, and in more than one sense.

  1. Chart.update doesn't work, but yAxis.update works: https://jsfiddle.net/49wefqok/ . My guess (I don't have time to follow up right now), is that Chart.update fails to relay the update to the yAxis object because it erroneously thinks that nothing is changed (because of undefined?).
  2. However, even when calling yAxis.update directly, the chart-level chart.options.yAxis[0] are not changed. It may be that we should mirror chart.options.yAxis[0] with the corresponding chart.yAxis[0].options. Though I am now sure about that one. At least, resolving the first problem will resolve the use case even though chart.options.yAxis is not in sync.

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented Nov 15, 2021

Further investigation leads me to the cleanRecursive function, where equal properties of two objects are removed. So when one setting in the newer object is explicitly set to undefined, and the reference object is also undefined (because it is left out), that setting is removed.

} else if (
isObject(newer[key]) ||
newer[key] !== older[key]
) {

Should be replaced with this:

        // Arrays, primitives and DOM nodes are copied directly
        } else if (
            isObject(newer[key]) ||
            newer[key] !== older[key] ||
            // If the newer key is explicitly undefined, keep it (#10525)
            (key in newer && !(key in older))
        ) {

@KacperMadej I'll mark this good for beginners, then it's up for grabs. Write a test, implement a fix and create a PR.

@TorsteinHonsi
Copy link
Collaborator

For the records: It is also a problem that chart.options.yAxis is not kept in sync with chart.yAxis[0].options. But I think that's a more complex issue, and if we wanted to fix it it would require a major refactoring.

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

Successfully merging a pull request may close this issue.

5 participants