Fix Array Knob deserialization #2217
Conversation
When an Array knob's value is serialized from the URL into the store, it is an Object. This ensures that when a user refreshes the page, the Array knob will properly deserialize the Object into an Array.
Codecov Report
@@ Coverage Diff @@
## master #2217 +/- ##
==========================================
+ Coverage 21.44% 21.48% +0.04%
==========================================
Files 263 263
Lines 5801 5804 +3
Branches 705 698 -7
==========================================
+ Hits 1244 1247 +3
- Misses 4016 4030 +14
+ Partials 541 527 -14
Continue to review full report at Codecov.
|
|
Thanks for the PR and the test! |
| ArrayType.deserialize = value => { | ||
| if (Array.isArray(value)) return value; | ||
|
|
||
| return Object.keys(value).reduce((array, key) => [...array, value[key]], []); |
There was a problem hiding this comment.
Does this rely on Object.keys being sorted? They are actually not guaranteed to be:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys
The Object.keys() method returns an array of a given object's own enumerable properties, in the same order as that provided by a for...in loop
There is no guarantee that for...in will return the indexes in any particular order
Please sort the keys before reducing
There was a problem hiding this comment.
@Hypnosphi you are correct, good catch. Will amend now.
Object.keys does not guarantee any particular order, which could result in an out-of-order array if we did not sort said keys.
The remaining expectations implicitly check whether the objects are arrays.
Issue: Array Knob data retrieved from the URL is stored as an Object
What I did
Updated the Array knob's
deserializemethod to handle Objects.How to test
Without these changes, if you reload the page with a component using the
arrayknob and expecting an Array; you'll get the errorIs this testable with jest or storyshots?
Yes; unit tests have been added for Array's
deserialize.Does this need a new example in the kitchen sink apps?
No.
Does this need an update to the documentation?
No.