added Object key sort prior to JSON.stringify comparison (fix #5908) #5993
Conversation
|
This maybe not working because AFAIK it's |
|
Definitely a somewhat trivial edge case. There are two ways to ensure this AFAIK. This approach has some reusability or you can pass sorted keys to stringify |
| it('Sorts an unsorted Object, which are equal', () => { | ||
| const unsortedTestObj = unsortedObject() | ||
| const sortedObject = keySort(unsortedTestObj) | ||
| expect(looseEqual(sortedObject, sortedObject)).toBe(true) |
There was a problem hiding this comment.
Either one should be unsortedTestObj here.
|
@drewsmith Is this right approach? If I compare key by key in two objects, it would take |
|
@znck Thanks for the comment. Are you referring to passing sorted keys to The right approach is deep object equality IMO, which |
|
I want to say, sorting the keys beats the purpose of Also, |
|
I agree with you depending on semantics. Does loose mean implicit deep copy, or is it a synonym for shallow? |
There was a problem hiding this comment.
The bottom line is to keep current semantic the same.
As the name looseEqual suggest, this function will not behave as desire 100%. ECMA spec requires no insertion ordering on Object enumeration nor JSON.stringify. Though most JS engine will keep the order so sorting can improve comparison if users compare {a: 1, b: 2} with {b:2, a:1}, where key insertion order is different.
Of course, a full implementation like https://github.com/substack/json-stable-stringify is too heavy for looseEqual which is only used in v-model.
| */ | ||
| export function keySort (obj: any): Object { | ||
| if (isObject(obj) === false) { | ||
| return {} |
There was a problem hiding this comment.
If obj isn't object, this function always return an empty object which makes such call like looseEqual(1, 2) returning true.
| } | ||
| const sorted = {} | ||
| Object.keys(obj).sort().forEach(key => { | ||
| sorted[key] = obj[key] |
There was a problem hiding this comment.
This only sorts the first level of an object. For nested object like {nested: {a: 1, b: 2}} and {nested: {b:2, a:1}}, it fails.
A recursive call should pay more attention to cyclic references.
| * by succinct key order. Guaranteed to at least return an | ||
| * empty object. | ||
| */ | ||
| export function keySort (obj: any): Object { |
There was a problem hiding this comment.
This function is only used in this file, consider not exporting it. Also, obj is annotated as any which in this case, Object is a better choice, if you perform no recursive call.
| }) | ||
|
|
||
| it('returns Object for Array argument', () => { | ||
| expect(looseEqual(keySort([]), {})).toBe(true) |
There was a problem hiding this comment.
This changes looseEqual's original behavior.
|
Maybe check how this package does it. |
|
Thanks for the PR - however, cloning with sorted keys results in extra memory allocation and it's better to just compare in place. See a8ac129 |
#5908
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
devbranch for v2.x (or to a previous version branch), not themasterbranchfix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information: