fix: handle empty data in column cells gracefully to avoid viz-switching runtime errors #71
Conversation
hydrosquall
changed the title
hydrosquall
force-pushed
the
cameron.yick/fix-viz-switching-bug
branch
2 times, most recently
from
261e24a
to
1b84808
Compare
…en finding unique values Previously, there would be runtime errors if users switched to columns with cell values that lacked a toString() method Now, the app does not crash when columns contain cell values that do not have toString() inside, such as "undefined" or "null"
hydrosquall
force-pushed
the
cameron.yick/fix-viz-switching-bug
branch
from
1b84808
to
a7f202e
Compare
| : uniques, | ||
| [], | ||
| ); | ||
| const uniqueValues = dim1 === "none" ? [] : getUniqueValues(sortedData, dim1); |
There was a problem hiding this comment.
If a user provides a dataset with a column called none, it's going to break other things in the data-explorer because none is already heavily used internally. If we rewrite this in the future, we may want to use a namespaced constant like $$_DX_NONE_$$ for our none column name to reduce the risk of collision with a user's dataframe. For now, I think it's out of scope to change that at the same time.
| @@ -20,7 +20,7 @@ export const sortByOrdinalRange = ( | |||
| rAccessor: string | (() => void), | |||
| secondarySort: string, | |||
| data: Dx.DataProps["data"], | |||
| ): any[] => { | |||
| ): Dx.DataProps["data"] => { | |||
There was a problem hiding this comment.
What goes in, comes out again
|
Thanks @hydrosquall. Tagging @emeeks @rgbkrk @MSeal since Cameron's use of replay is interesting. Cameron, how are you liking replay? |
|
Hey! Jason from the replay.io team. Happy to answer any questions as well :) |
@willingc I like using it! I'm trying to use it as much as I can when doing open source JS work that doesn't involve a WebGL component, as that isn't supported yet. The biggest advantage vs just capturing a GIF or video is that other reviewers can explore a captured sequence application state without needing to spin up the app themselves, and add logging statements to surrounding files as necessary. This particular bug was a bit of a simplistic example since triggering the bug only required 1 click to trigger, but I can see this being even more valuable to demonstrate bugs with trickier reproduction paths. I think the "commenting" feature to collaboratively discuss bugs synced with particular moments in time has potential, but I haven't used this feature extensively. |
|
@jasonLaster Thanks for offering to answer questions. If a team is already using Cypress, what are the best practices to add replay.io? |
|
I'm going to merge this PR for the purpose of removing the original runtime bug, but feel free to continue discussing From what I gather using Replay.io wouldn't directly affect a team's usage of Cypress, since Cypress would be for running programmatic tests as part of CI, whereas |
|
🚀 PR was released in |
|
@willingc yep - seconding what @hydrosquall said. Replay.io is great when folks want to record a bug and share it with others |
Motivation
Changes
JSON.stringifyinstead of.toString(), this handlesnullandundefinedgracefully (the previous error happens because those empty values don't have atoString()attached. We technically have the option of ignoring those values entirely, but it might be unexpected behavior depending on the user.Setto find unique values in each column rather than looping through the entire array twice as the previousfind/reducemechanism was doing.Testing
Repro instructions: load the codepen, and start switching visualization types
Note: switching to the line chart triggers a different sort of error, but I'll save that for a different investigation.
(GIF: https://a.cl.ly/qGu5nReq )
📦 Published PR as canary version:
8.2.12--canary.71.a7f202e.0✨ Test out this PR locally via: