Skip to content
This repository has been archived by the owner on Mar 24, 2024. It is now read-only.

Patch chart.js to avoid providing invalid options to the NumberFormatter #5786

Merged

Conversation

defunctzombie
Copy link
Contributor

User-Facing Changes
Fixes a crash in the plot panel when working with Float64 max values.

Description
When encountering datasets with large numbers (like Number.MAX_VALUE), the tick claculation
logic produces NaN. These NaN values would then get used to create NumberFormatter with min/max fraction digits.

DOM interfaces do not allow using NaN for these options and will throw. This change checks for NaN prior to building
the options for the formatter and uses a safe value instead of NaN to avoid throwing and crashing the panel.

When encountering datasets with large numbers (like Number.MAX_VALUE), the tick claculation
logic produces NaN. These NaN values would then get used to create NumberFormatter with min/max fraction digits.

DOM interfaces do not allow using NaN for these options and will throw. This change checks for NaN prior to building
the options for the formatter and uses a safe value instead of NaN to avoid throwing and crashing the panel.
@defunctzombie
Copy link
Contributor Author

Not sure how I feel about this patch but it does seem to do the trick to avoid a crash.

.gitignore Outdated
@@ -13,6 +13,7 @@ storybook-static/
!.yarn/releases
!.yarn/plugins
!.yarn/sdks
!.yarn/patches
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep it in the same place with our other patches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - somehow I thought this was the place but I see now that this is where yarn put it after I ran the command yarn told me to run.

Maybe we should move it here since this is where yarn put the patch after running yarn commit-patch ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I suppose moving all of them here could make sense if that is the default location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of changing our approach, I've updated the yarn config to set the patch directory to our existing directory: https://github.com/foxglove/studio/pull/5786/files#diff-88fbe28c4102501b94961511a0d70ff895bf39970b4d3fc11917794a239117c5R9

Now when you use yarn commit-patch you get a file in the existing patches directory.

@defunctzombie defunctzombie merged commit b39478f into main Apr 19, 2023
14 checks passed
@defunctzombie defunctzombie deleted the roman/fg-2966-minimumfractiondigits-value-is-out-of branch April 19, 2023 15:54
@@ -6,6 +6,8 @@ defaultSemverRangePrefix: ""

nodeLinker: node-modules

patchFolder: ./patches
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? Make it automatically put patches there when using -s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep - when you do yarn patch, yarn gives you a command to run to finalize the patch. This is the folder where it puts the patch it creates from your changes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants