Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

order with the default comparator #55

Merged
merged 1 commit into from Sep 10, 2019

Conversation

@rickbutton
Copy link
Collaborator

commented Sep 10, 2019

@rickbutton rickbutton requested a review from rricard Sep 10, 2019

@ljharb
ljharb approved these changes Sep 10, 2019

@rickbutton rickbutton merged commit 722f246 into master Sep 10, 2019

@rricard rricard deleted the rb/sort-default-comparator branch Sep 10, 2019

@littledan

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

I don't think the default comparator is exactly what we want here. Don't we want all of the indexed properties to come first, like in a normal object? Also, doesn't the default comparator throw on Symbols, which may be present as property keys?

@rickbutton

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

@littledan thats a good question! The proposal originally mimic-ed the existing object behavior of indexed properties coming first, but I simplified it. I have no opinion either way, I imagine your argument for it is consistency with how objects work?

Also very good point, the default comparator will throw for symbols. This isn't a problem for Object.keys or like for...in in general, because symbols don't appear in that enumerated list of properties. I imagine it would stay the same for records? If not, what behavior would you expect? Would symbols appear in the list of properties according to their description string?

@ljharb

This comment has been minimized.

Copy link

commented Sep 11, 2019

Perhaps, instead of array sort, the order of Reflect.ownKeys should be used?

@littledan

This comment has been minimized.

Copy link
Collaborator

commented Sep 11, 2019

@ljharb I think that would be insertion order (modulo the integer key sorting thing I mentioned earlier), which is exactly what we don't have.

Let's discuss this in an issue. I think this could be a bit of a more detailed discussion. We really just need something well-defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.