-
Notifications
You must be signed in to change notification settings - Fork 62
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
Expand R&T support across methods that accept objects and arrays #264
Conversation
Do you think that these should be valid? new Map([ #{ 0: "x", 1: "y", length: 2 } ]);
new Map([ #{ 0: "x", 1: "y", length: 1 } ]);
fn.apply(thisArg, #{ 0: "x", 1: "y", length: 2 }); |
AddEntriesFromIterable doesn't currently require a Since we're adding support for tuple, and these already support array like objects, why not add array like records support as well? |
Most places only accept an arraylike, or only an iterable; |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stopped partway through, but overall it seems like it'd be better to avoid the frequent object coercion if possible
spec/miscellaneous-updates.html
Outdated
1. <ins>If Type(_nextItem_) is Tuple, then</ins> | ||
1. <ins>Set _nextItem_ to ! ToObject(_nextItem_).</ins> | ||
1. <ins>Else</ins> if Type(_nextItem_) is not Object, then | ||
1. Let _error_ be ThrowCompletion(a newly created *TypeError* object). | ||
1. Return ? IteratorClose(_iteratorRecord_, _error_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this just work the same without the object conversion? ie "if it's not tuple or object", and then later, the Get will Just Work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get
expects an object: https://tc39.es/ecma262/#sec-get-o-p
The alternatives are:
- Special-case records and tuples in
Get
(I cannot use_record_.[[Get]]
, since I think primitives don't have internal methods?) - Use
GetV
, which is justToObject
+Get
.
(same for LengthOfArrayLike
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Special-casing Get and friends to handle records and tuples seems like a reasonable approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the whole spec, Get()
is used ~140 times on objects. We would need to call it on tuples only ~10 times, so I think it's better to use GetV
when dealing with "R&T or objects", making it easier to see "this will only be called with objects, thus we can make these assumptions/optimizations" in the majority of cases while reading the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about future edits? It seems like it'd be an easy mistake to make to use Get with an R&T and have it break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be as hard/easy as it is to currently accidentally use primitive strings where we expect array-like values. When we are not sure that we have an object, we should use GetV
"by defaut" in any algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure i see that - records are object-like and tuples are array-like, conceptually, and strings being actually arraylike doesn't mean they're conceptually arraylike :-)
@mhofman ah, i see what you mean. yes, that's true, it seems reasonable for either Tuples or Records to be the entry objects in Map/Set/Object.fromEntries. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@@ -873,20 +873,17 @@ <h1> | |||
</emu-alg> | |||
</emu-clause> | |||
|
|||
<emu-clause id="sec-isconcatspreadable" type="abstract operation"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviewers: isconcatspreadable
not deleted but moved to miscellaneous-updates.html
I don't think |
Isn't tuple array-like? |
It is 100% arraylike; i fail to see why it wouldn’t qualify. |
Hi @michaelficarra , would you be able to expand on this? What would your expectation be for the following: let a = ['a1', 'a2'];
let b = #['b1', 'b2'];
a.concat(a); // ['a1', 'a2', 'a1', 'a2'];
a.concat(b); // ?
b.concat(a); // ?
b.concat(b); // ? |
Yes array-like in that it has a numeric length and numeric indices, but not meeting the bar for
let a = ['a1', 'a2'];
let b = #['b1', 'b2'];
a.concat(a); // ['a1', 'a2', 'a1', 'a2']
a.concat(b); // ['a1', 'a2', #['b1', 'b2']]
b.concat(a); // whatever you like
b.concat(b); // #['b1', 'b2', 'b1', 'b2']; |
I agree that concat doesn’t spread all arraylikes - but Tuple is far closer to an array than most arraylikes, as a conceptual and iterable List, with list operations on its prototype. |
This is far too weak of a statement. It spreads no array-likes. It is meant to only spread arrays. For historical HTML integration reasons, we had to add the |
Yes, it only spreads Arrays and NodeLists, but Tuples are also built-in list-like collections. Why wouldn’t we want to expand it further? Array concat’s flattening behavior is immensely idiomatic and useful. |
I'd argue that |
That’s also why a.concat(b) is so common - the hope that if a list, it will be spread, and if not, not. Because spreading syntax also spreads strings, prior to flat existing, concat was the only way to do this, and as such it’s quite common to do so. |
There are other places where we are treating Tuples more like Arrays than 'array-like'. Namely |
Additionally, |
@ljharb Nothing requires |
I think user expectations do. We added a bunch of methods to Array just so Arrays and Tuples would be consistent. If arrays can spread Arrays and NodeLists, Tuples must be able to as well, but the same logic. |
Ehhh.... NodeLists contain non-primitives pretty much by definition. I am entirely OK with Tuple.prototype.concat rejecting empty NodeLists in addition to non-empty NodeLists (which it must anyway). |
Co-authored-by: Jordan Harband <ljharb@gmail.com>
- R&T are stringified as objects and arrays - The replacer argument can be a tuple
3a3b146
to
71c3e41
Compare
I didn't add support for:
Reflect.*
, since they already disallow primitives even if it would make sense to use them (e.g.Reflect.get("str", 0)
).new Proxy(target, #{})
), since it's meant to contain functions so a record would only work as an empty handler.EDIT: done.JSON.stringify
's replacer array, just to avoid conflicts with Update boxes to work in SES environments #257: I'll do it in a separate PR in the futureThere are also a bunch of bugs in the current JSON implementation, where we pass R/T to AOs that expects objects.Closes #107
This PR should be reviewed commit-by-commit. Most algorithms in the
misc-updates.html
file are copied from the existing spec (except forInitializeTypedArrayFromTuple
andUnwrapIteratorResult
): focus on the<ins>
and<del>
tags.