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

Fix "Rendered more hooks than during the previous render." in LineGraph #8316

Merged
merged 2 commits into from
Jan 28, 2022

Conversation

Twixes
Copy link
Collaborator

@Twixes Twixes commented Jan 27, 2022

Changes

I was getting these errors occasionally within LineGraph:
Screen Shot 2022-01-27 at 16 43 08

It appears the cause is LineGraph returning early (i.e. before all hooks run), which is prone to some race condition causing it NOT to return early at times, in which case all the V2 hooks get to run and make React extremely confused. The solution is to just refactor the LineGraph version-choosing logic into a component of it's own that just renders the right actual component.

@Twixes
Copy link
Collaborator Author

Twixes commented Jan 27, 2022

Hmm, looks like there are two crashes possible:

  1. The feature flags case, fixed
  2. A bug in thechartjs-plugin-crosshair Cannot read property 'dragStarted' of undefined AbelHeinsbroek/chartjs-plugin-crosshair#10, not sure how to fix yet

@Twixes Twixes marked this pull request as draft January 27, 2022 16:54
@Twixes
Copy link
Collaborator Author

Twixes commented Jan 27, 2022

I believe problem 2. is a race condition between when the chart is generated and when it's sized, but not sure where exactly does this occur. However, this seems to occur only in InsightCard AND appears to be fixed by #8302 – specifically, the new InsightViz sizing mechanism. Therefore this PR is viable on its own.

@Twixes Twixes marked this pull request as ready for review January 27, 2022 18:36
Copy link
Contributor

@alexkim205 alexkim205 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this :) this works great

@@ -17,7 +17,7 @@ import {
TooltipModel,
TooltipOptions,
} from 'chart.js'
import { CrosshairOptions, CrosshairPlugin } from 'chartjs-plugin-crosshair'
import CrosshairPlugin, { CrosshairOptions } from 'chartjs-plugin-crosshair'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oo good catch

@alexkim205 alexkim205 merged commit 4b37908 into master Jan 28, 2022
@alexkim205 alexkim205 deleted the fix-line-graph-v2-hooks branch January 28, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants