This repository was archived by the owner on Apr 12, 2024. It is now read-only.
fix(orderBy): do not try to call valueOf/toString on null
#10386
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CLAs look good, thanks! |
|
there are other ways to fix this, but this seemed the most correct. It's really hard to tell though. @petebacondarwin can you review? |
| { id: 3 }, | ||
| null | ||
| ]; | ||
| expect(orderBy(array)).toEqualData(array.sort()); |
There was a problem hiding this comment.
of course, can't rely on IE to behave similarly to other browsers here =( whelp, fixing the tests I guess.
8bfeddb added changes to make relational operator work as it normally would in JS --- unfortunately, this broke due to my failure to account for typeof null being "object". This refactoring attempts to convert object values to primitives still, in a fashion similar to the SortCompare (and subsequently the ToString() algorithm) from ES, in order to account for `null` and also simplify code to some degree. BREAKING CHANGE: Previously, if either value being compared in the orderBy comparator was null or undefined, the order would not change. Now, this order behaves more like Array.prototype.sort, which by default pushes `null` behind objects, due to `n` occurring after `[` (the first characters of their stringified forms) in ASCII / Unicode. If `toString` is customized, or does not exist, the behaviour is undefined. Closes angular#10385
|
This seems cleaner. I guess all the tests pass in which case LGTM |
|
alright thanks |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
8bfeddb added changes to make relational operator work as it
normally would in JS --- unfortunately, this broke due to my failure to account for typeof null
being "object".
This refactoring attempts to convert object values to primitives still, in a fashion similar to
the SortCompare (and subsequently the ToString() algorithm) from ES, in order to account for
nulland also simplify code to some degree.
BREAKING CHANGE:
Previously, if either value being compared in the orderBy comparator was null or undefined, the
order would not change. Now, this order behaves more like Array.prototype.sort, which by default
pushes
nullbehind objects, due tonoccurring after[(the first characters of theirstringified forms) in ASCII / Unicode. If
toStringis customized, or does not exist, thebehaviour is undefined.
Closes #10385