React JSX Highcharts v4 Alpha Feedback (Hooks rewrite)

whawker commented 5 years ago

This issue exists to feedback any issues discovered while testing the new version 4 alphas.

Please note, this rewrite is based on React Hooks, so requires React 16.8 as minimum. Please see the release notes for more information on breaking changes.

leonardolessa commented 5 years ago

I had an issue trying to updating chart series while rendering a PlotLine:

Uncaught TypeError: axis.removePlotLine is not a function at usePlotBandLineLifecycle.js:63 at commitHookEffectList (react-dom.development.js:21073) at commitPassiveHookEffects (react-dom.development.js:21106) at HTMLUnknownElement.callCallback (react-dom.development.js:363) at Object.invokeGuardedCallbackDev (react-dom.development.js:412) at invokeGuardedCallback (react-dom.development.js:466) at flushPassiveEffectsImpl (react-dom.development.js:24196) at unstable_runWithPriority (scheduler.development.js:674) at runWithPriority$2 (react-dom.development.js:11834) at flushPassiveEffects (react-dom.development.js:24167) at react-dom.development.js:23728 at scheduler_flushTaskAtPriority_Normal (scheduler.development.js:450) at flushTask (scheduler.development.js:503) at flushWork (scheduler.development.js:635) at performWorkUntilDeadline (scheduler.development.js:237) at onTimeout (scheduler.development.js:289)

I'll try to make an stackblitz of it as soon as I can but right now I'm a little bit busy, I hope this can be useful.

anajavi commented 5 years ago

@leonardolessa don't bother with the stackblitz. I already found the bug. It should be axis.removePlotBandOrLine not removePlotLine.

Thank you very much for trying out the alpha!

edit: fixed and added test in https://github.com/whawker/react-jsx-highcharts/pull/216/commits/41c028d17525367315b7db2471dd185e7a9ea531

anajavi commented 5 years ago

@whawker Can you make a new alpha release when you have time? There's a couple of fixes and optimizations

whawker commented 5 years ago

Yep of course.

FYI The funeral was yesterday, so I should be more available to assist from now on.

anajavi commented 5 years ago

Sorry to hear about your loss. Take your time.

whawker commented 5 years ago

Thanks @anajavi

Published:

react-jsx-highcharts@4.0.0-alpha.2
react-jsx-highstock@4.0.0-alpha.2
react-jsx-highmaps@2.0.0-alpha.2

All using NPM tag next

whawker commented 5 years ago

Just looking into an issue with RangeSelector inputs and buttons for the Highstock package, they don't appear to be displaying in my tests.

I think it's something to do with the rendered state not setting properly? But it could just be a red herring as both RangeSelectorInput and RangeSelectorButton return null, so probably aren't displayed in React Dev Tools

anajavi commented 5 years ago

RangeSelectorInner never gets axis from useAxis because it's not inside Axis or provide axisId for useAxis. That's why it does not render.

anajavi commented 5 years ago

RangeSelector fixed in ed43336bd916c9ac18b0d62b65a8aab667a710fd and 1df5ca3e2fdcc6e5ef4c54e435d31fa1799e54f2

whawker commented 5 years ago

Oh wow, nice one, thanks!

jhartling commented 5 years ago

I've run in to an issue with a highcharts plugin for draggable plot lines.

Working in latest: https://stackblitz.com/edit/base-chart-example-xsurfm?file=MyChart.js

Broken in next: https://stackblitz.com/edit/base-chart-example-olzxfe?file=MyChart.js

The line is holding on to the initial value in next.

anajavi commented 5 years ago

I've run in to an issue with a highcharts plugin for draggable plot lines.

Working in latest: https://stackblitz.com/edit/base-chart-example-xsurfm?file=MyChart.js

Broken in next: https://stackblitz.com/edit/base-chart-example-olzxfe?file=MyChart.js

The line is holding on to the initial value in next.

The Plotline seems to have broken update lifecycle. I will fix this.

anajavi commented 5 years ago

@jhartling PlotLine lifecycle fixed in https://github.com/whawker/react-jsx-highcharts/pull/216/commits/3b318deb3979301479eb508ee24a3062558a82ec

anajavi commented 5 years ago

@jhartling I modified your Highcharts plugin to return plotline created. It now works with alpha.4 https://stackblitz.com/edit/base-chart-example-uwibmf?file=MyChart.js

anajavi commented 5 years ago

if we export usePlotline, the draggable plotline could be implemented cleanly in react component without patching Highcharts.

non-working example:

const MyDraggablePlotLine = props => {
  const plotline = usePlotLine();
  draggablePlotLine(plotline);
}

usage:

<PlotLine value={220} width={10}>
  <MyDraggablePlotLine />
</PlotLine>

jhartling commented 5 years ago

@anajavi Awesome, thanks for your work on this great library!

anajavi commented 5 years ago

@whawker how do you feel about prop-types? I don't know if they add any value now that the HOCs are removed.

whawker commented 5 years ago

I'm in two minds, largely anything you can do with the API outlined at https://api.highcharts.com you can do with our library, so maybe we can just refer people there.

We do have the noticeable differences though, in regard to "text as children" and events as onEventName, both of which we mention in the Readme. Do we feel that is sufficient?

Maybe we could drop prop types and add more notices to the developer, like we do for missing Highcharts modules? For instance, if we see an events prop, have a notice (or error) that says we require onEventName?

anajavi commented 5 years ago

Removing the prop types is not terribly important, and I don't think it is breaking change. So it doesn't need to be decided now.

whawker commented 5 years ago

Just noting a few more issues the alphas, so they don't get missed.

The plot bands in the Highstock navigator are no longer displayed, see previous here
The animation in the solid gauge example, seems to gone a little awry, see previous here
The custom component example should be updated to use the exposed hooks

And obviously, there is a ton of documentation in the wiki and Readmes to update.

anajavi commented 5 years ago

The animation in the solid gauge example, seems to gone a little awry, see previous here

This is side-effect of replacing isEqual with Object.is. The example works by modifying the example to:

const dataLabels = {
  format: '<div style="text-align:center"><span style="font-size:25px;color:black">{y}</span><br/><span style="font-size:12px;color:silver">km/h</span></div>',
  y: -50
};
const tooltip = {
  valueSuffix: ' km/h'
}

<SolidGaugeSeries
  name='Speed'
  data={[ this.state.kmph ]}
  dataLabels={dataLabels}
  tooltip={tooltip}
/>

Without constant objects in the props the Series-component keeps calling series.update, which seems to cause re-animating the gauge.

The custom component example should be updated to use the exposed hooks

We could make an example of the draggableplotline being mounted with usePlotBandLine?

anajavi commented 5 years ago

The plot bands in the Highstock navigator are no longer displayed, see previous here

Fixed in https://github.com/whawker/react-jsx-highcharts/commit/9e3ccd29bc47f92140925d785453153d1af36729

anajavi commented 5 years ago

Now SplineWithPlotBands example does not work when using minified production build. It works with non-minified development mode build. I suspect there is something wrong with the minifier.

anajavi commented 5 years ago

SplineWithPlotBands fixed in https://github.com/whawker/react-jsx-highcharts/commit/abcdbbe2da55bb79def49fb375ee5e31a239be72

whawker commented 5 years ago

Look like I'm going to have to revert this commit as they are used by the other packages to test the components.

https://github.com/whawker/react-jsx-highcharts/blob/master/packages/react-jsx-highstock/test/components/RangeSelector/RangeSelector.spec.js#L23-L29 https://github.com/whawker/react-jsx-highcharts/blob/master/packages/react-jsx-highmaps/test/components/HighchartsMapChart/HighchartsMapChart.spec.js#L16-L18

anajavi commented 5 years ago

Look like I'm going to have to revert this commit as they are used by the other packages to test the components.

I can rewrite those tests without the contexts. It can be done by mocking the hooks instead.

Sorry for the trouble, I forgot to check the tests.

anajavi commented 5 years ago

Look like I'm going to have to revert this commit as they are used by the other packages to test the components.

Tests were rewritten to mock hooks in https://github.com/whawker/react-jsx-highcharts/commit/9d9685961fc71975738210c34a4023f0e5d294a5

anajavi commented 5 years ago

About the docs.

How about trying out docusaurus. It can pull markdown docs from github and apparently it can also deploy to gh-pages. Used by quite many projects. It might even be able to embed stackblitz examples, but I haven't tried.

whawker commented 5 years ago

I've rewritten the Custom component example using our exposed hooks - it works really nicely! Much easier than before.

(Re: Solid gauge example) This is side-effect of replacing isEqual with Object.is.

Interesting, might need to give this one some thought, I fear we might get quite a few issues raised for these types of things.

Obviously avoiding the creation of objects in render is best practice, but I don't believe many people actually follow this particular practice.

About the docs.

docusaurus look good, just having a read of the how-tos as we speak.

anajavi commented 5 years ago

I've rewritten the Custom component example using our exposed hooks - it works really nicely! Much easier than before.

I agree. I also really like the logic sharing that hooks give.

Do the examples live in that repository nowadays? What about the examples in this repository?

note: You might want to add useCallback to handleFromDateChange to prevent re-renders of DayPicker.

(Re: Solid gauge example) This is side-effect of replacing isEqual with Object.is.

Interesting, might need to give this one some thought, I fear we might get quite a few issues raised for these types of things.

Obviously avoiding the creation of objects in render is best practice, but I don't believe many people actually follow this particular practice.

This is true. With Object.is it is however possible to optimize those re-renders. With very large datasets the re-renders caused lots of expensive calls to isEqual (same data is the most expensive to check).

About the docs.

docusaurus look good, just having a read of the how-tos as we speak.

The react-leaflet is same kind of wrapper for Leaflet as this project is for Highcharts. It uses docusaurus, so maybe same kind of format would work there? Especially linking from component docs to Highcharts docs about the options.

anajavi commented 5 years ago

I've rewritten the Custom component example using our exposed hooks - it works really nicely! Much easier than before.

Isn't that example actually the same as the whole project of react-jsx-highstock-datepickers? Nicely done indeed!

whawker commented 5 years ago

Do the examples live in that repository nowadays? What about the examples in this repository?

The examples in each are basically the same - the ones in this repo are static generated, so we can display them on https://whawker.github.io/react-jsx-highcharts/examples - whereas the other repo is a create-react-app so I can test everything works properly in a real life use case.

react-leaflet and docusaurus

Good idea, big fan of that project (although I have never had cause to use it) - I liked how they attacked their problem in a similar way to ours - until the hooks rewrite anyway!

Isn't that example actually the same as the whole project of react-jsx-highstock-datepickers?

Basically yes, although that project allowed for a bit more customisation.

whawker commented 5 years ago

@anajavi just pushed a docusaurus branch with just the website so far. Let me know your thoughts?

Also includes a potential new logo?

anajavi commented 5 years ago

Whoaa! A Really, really nice site you made!

I quickly tried to embed stackblitz with an iframe in the site, and it kind of works too.

Really impressed, nice work @whawker !

jhartling commented 5 years ago

I'm hitting some issues in the alpha with the Navigator component.

Resizing the navigator produces weird behaviour like it's changing the axis type / date range before ultimately freezing. It's also throwing an error when updating Navigator props.

anajavi commented 5 years ago

@jhartling can you please paste the error here?

jhartling commented 5 years ago

@anajavi

highstock.js:11462 Uncaught TypeError: Cannot read property 'breaks' of undefined
    at D.init (highstock.js:11462)
    at D.update (highstock.js:11281)
    at Object.<anonymous> (highstock.js:8524)
    at p (highstock.js:115)
    at c.Chart.update (highstock.js:8523)
    at updateNavigator (Navigator.js:96)
    at Navigator.js:81

highstock.js:128 Uncaught Error: Highcharts error #18: www.highcharts.com/errors/18/
    at c.Chart.r (highstock.js:128)
    at Object.c.fireEvent (highstock.js:625)
    at Object.c.error (highstock.js:142)
    at highstock.js:7255
    at Array.forEach (<anonymous>)
    at d.<anonymous> (highstock.js:7250)
    at c.fireEvent (highstock.js:625)
    at d.bindAxes (highstock.js:7249)
    at d.init (highstock.js:7202)
    at d.k.init (highstock.js:13887)
    at d.update (highstock.js:8725)
    at Series.js:144

index.js:1437 The above error occurred in the <Navigator> component:
    in Navigator (at Chart/index.js:178)
    in div (created by BaseChart)
    in BaseChart (created by HighchartsStockChart)
    in HighchartsStockChart (at Chart/index.js:161)
    in div (at Chart/index.js:129)
    in Chart
    in Chart (created by HighchartsWrappedComponent)
    in HighchartsWrappedComponent (at ChartWidget/index.js:656)

anajavi commented 5 years ago

@whawker what do the following lines in navigator do?

https://github.com/whawker/react-jsx-highcharts/blob/f530ee83dce15788f03eaee9eeeff87b9b8e1cb6/packages/react-jsx-highstock/src/components/Navigator/Navigator.js#L51-L57

I think the cloning might somehow make the navigator children to refer to wrong axis, but I am not sure.

anajavi commented 4 years ago

Navigator problem turned out to be a little more complex than simply cloning elements. The problem exists to a lesser degree in react-jsx-highstock@3 too.

Updating navigator destroys it first, and then runs init: https://github.com/highcharts/highcharts/blob/426ec31921b4e7d65678ef64f0e6753ee1404ef5/js/parts/Navigator.js#L828-L839

Init then recreates the navigator axis: https://github.com/highcharts/highcharts/blob/426ec31921b4e7d65678ef64f0e6753ee1404ef5/js/parts/Navigator.js#L1276-L1297

After the navigator axis is recreated, the <NavigatorXAxis> component holds stale reference to axis that does not exist anymore. That's why it is throwing errors.

This can be fixed by always getting the newest axis object from Highcharts before updating it:

const NavigatorAxis = ({ children, axisId, ...restProps }) => {
  const chart = useChart();
  const renderedRef = useRef(false);
  useEffect(() => {
    const axis = chart.get(axisId);
    if (!axis) return;

    updateNavigatorAxis(getNonEventHandlerProps(restProps), axis);
  }, [chart]);

Also NavigatorAxis should create AxisContext to pass axis to its children.

anajavi commented 4 years ago

While trying to fix Navigator component, I found a couple of other issues in Highstock navigator: https://github.com/highcharts/highcharts/issues/12406

whawker commented 4 years ago

Apologies for the lack of input here, life has been massively hectic the past few weeks.

Thanks for looking into the navigator issues. It looks as though cloneElement can be deleted, it's left over from some poor refactoring from v2 to V3 when we used to pass down seriesCount to the child components. https://github.com/whawker/react-jsx-highcharts/blob/2.2.1/packages/react-jsx-highstock/src/components/Navigator/Navigator.js#L27

anajavi commented 4 years ago

I've tried to fix the navigator in various ways. But it seems that getting navigators xAxis to work consistently is hairy.

The navigator provides usable xAxis when navigator is first created. Any update to navigator causes the navigator axis to be recreated. After recreation trying to get the new axis instance from Highcharts is not consistent. Might be timing related problem.

anajavi commented 4 years ago

I think I found a way to make navigator work.

The fix is going to touch almost all parts of the code that deals with axes. Also I think axis and series updates needs to be changed so that they are executed on component render instead of in useEffect.

I am not entirely sure that the navigator can be fixed for all use cases, but let's see..

anajavi commented 4 years ago

v4.0.0-alpha.8 has the following new features:

The ColorAxis component

<ColorAxis minColor="#FAA" maxColor="#AAF">
<Series data={..} />
</ColorAxis>

Chart eventhandlers can now be updated


let randomString=Math.random().toString(36)

<Chart onClick={(event) => {console.log(randomString)}} />

The above is bad practice though, but sometimes needed.

@jhartling the navigator fix is in v4.0.0-dev.1. It's still has some rough edges and one hard to trigger bug. If you like to test it, be sure to check that both react-jsx-highcharts and react-jsx-highstock are the same version by doing:
```shell
$ npm ls react-jsx-highcharts ; npm ls react-jsx-highstock                                                                  
├── react-jsx-highcharts@4.0.0-dev.1 
└─┬ react-jsx-highstock@4.0.0-dev.1
  └── react-jsx-highcharts@4.0.0-dev.1

anajavi commented 4 years ago

The v4.0.0-dev.1 ended up being too broken. Strangely enough I can't anymore reproduce the Navigator crash with v4.0.0-alpha.8.

whawker commented 4 years ago

Published

react-jsx-highcharts 4.0.0 react-jsx-highstock 4.0.0 react-jsx-highmaps 2.0.0

Although I forgot to update the Readme's doing that now

anajavi commented 4 years ago

I added release notes draft.

Maybe we should close this issue and open a new feedback issue for the final release?

whawker commented 4 years ago

This issue is superseded by https://github.com/whawker/react-jsx-highcharts/issues/259

whawker commented 4 years ago

Clearly we think very alike! @anajavi I left you a note on your release notes draft.

whawker / react-jsx-highcharts

React JSX Highcharts v4 Alpha Feedback (Hooks rewrite) #228