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 Maximum call stack size exception in computeLabelSizes #7883

Merged
merged 1 commit into from Oct 17, 2020

Conversation

@silentmatt
Copy link
Contributor

@silentmatt silentmatt commented Oct 14, 2020

Calling Math.max with a large number of values was throwing an exception.
Tracking the current largest width and height as the widths and heights are
built up should be faster and avoids the exception.

)

Calling Math.max with a large number of values was throwing an exception.
Tracking the current largest width and height as the widths and heights are
built up should be faster and avoids the exception.
@silentmatt
Copy link
Contributor Author

@silentmatt silentmatt commented Oct 14, 2020

The same change could be made in the master branch, but I wasn't able to reproduce #7881. Something seems to have changed regarding the tickSize option, which might have been avoiding the issue.

@etimberg etimberg requested a review from kurkle Oct 15, 2020
kurkle
kurkle approved these changes Oct 17, 2020
Copy link
Collaborator

@kurkle kurkle left a comment

Maybe we should do this for v3 too.

@etimberg
Copy link
Member

@etimberg etimberg commented Oct 17, 2020

Would be a good idea to take this into v3 as well

@kurkle kurkle changed the title Fix Maximum call stack size exception in computeLabelSizes (#7881) Fix Maximum call stack size exception in computeLabelSizes Oct 17, 2020
@kurkle kurkle merged commit 42ed589 into chartjs:2.9 Oct 17, 2020
2 checks passed
@silentmatt silentmatt deleted the 2.9 branch Oct 17, 2020
@silentmatt
Copy link
Contributor Author

@silentmatt silentmatt commented Oct 17, 2020

I should be able to submit a PR for v3 soon. I figured I'd wait for this one to be approved first.

@etimberg
Copy link
Member

@etimberg etimberg commented Oct 17, 2020

thanks @silentmatt

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

Successfully merging this pull request may close these issues.

None yet

3 participants