-
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
Initial Record/Tuple standard library spec #131
Conversation
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 really seems like we'll have to rename the existing concept in the spec of "Record" before landing this proposal. What are the thoughts/plans around that?
I agree, we're tracking that in #96 but maybe we need to work on that earlier because the html output is very misleading right now |
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.
This all looks generally reasonable, though I haven't reviewed every word extremely closely. I guess you're positing two operations here:
-
CreateTupleFromList
This throws an exception if List has a non-primitive in it.
I think we might want to move responsibility for checking for non-primitives to the caller. And in general, do this more eagerly, e.g., formap
, check each time something is output.
In the context of Record and Tuple object model spec #132 , we might write this as, "A new Tuple value whose [[Sequence]] is list." (The JS spec generally tries to avoid unnecessary indirection for things like construction.) -
CreateListFromTuple
This needs returns a fresh copy of the list, as sometimes the caller modifies it.
In the context of Record and Tuple object model spec #132, we may instead write this as "tuple's [[Sequence]] value". In this case, we'd want to make a fresh copy of the list before mutating it.
1. Let _items_ be a List whose first element is _T_ and whose subsequent element are, in left to right order, the arguments that were passed to this function invocation. | ||
1. Repeat, while _items_ is not empty, | ||
1. Remove the first element from _items_ and let _E_ be the value of the element. | ||
1. Let _spreadable_ be ? IsConcatSpreadable(_E_). |
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 guess we should make sure that Tuples are concat spreadable (either by updating that algorithm or giving them the symbol; I prefer the former).
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's a protocol; it's fine to have default behavior here, but I think the symbol needs to exist and have its value respected.
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 what you're trying to say here. Nothing in the JS spec has that symbol defined. I'm not saying that we would stop querying the symbol, just that it would have this default behavior built into the IsConcatSpreadable algorithm, like Arrays do.
@littledan on |
@rickbutton I'm suggesting you replace the usages with the verbiage I mentioned. |
spec/immutable-data-structures.html
Outdated
1. Let _name_ be ? GetV(_obj_, `"0"`). | ||
1. Let _value_ be ? GetV(_obj_, `"1"`). |
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.
1. Let _name_ be ? GetV(_obj_, `"0"`). | |
1. Let _value_ be ? GetV(_obj_, `"1"`). | |
1. Let _name_ be ! GetV(_prop_, `"0"`). | |
1. Let _value_ be ! GetV(_prop_, `"1"`). |
(the !
is since we know these can't fail, because of EnumerableOwnPropertyNames
's 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.
Not sure why this was resolved
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.
apologies, part of my broken push, I've pushed a commit that contains all of @ljharb 's suggested changes. Should be good now.
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 the Tuple methods that make a new Tuple (map/filter/concat/spliced/etc), it seems like rather than duplicating the array method steps and adjusting them, there should be new abstract operations that both the Array methods and the Tuple methods use.
Co-authored-by: Jordan Harband <[email protected]>
1. Let _adder_ be a new Abstract Closure with parameters (_key_, _value_) that captures _fields_ and performs the following steps when called: | ||
1. If Type(_value_) is Object, throw a *TypeError* exception. | ||
1. Let _field_ be { [[Key]]: _key_, [[Value]]: _value_ }. | ||
1. Append _field_ to the end of list _fields_. | ||
1. Perform ! AddEntriesFromIterable(`undefined`, _iterable_, _adder_). |
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.
this looks much better, thanks!
Co-authored-by: Jordan Harband <[email protected]>
Oh, I realize what happened @ljharb , I messed up a force-push. Whoops! I will reapply your suggestions asap. |
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.
Overall, this seems good to land.
spec/immutable-data-structures.html
Outdated
1. Let _name_ be ? GetV(_obj_, `"0"`). | ||
1. Let _value_ be ? GetV(_obj_, `"1"`). |
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.
Not sure why this was resolved
1. Let _T_ be ? thisTupleValue(*this* value). | ||
1. Let _list_ be a new List containing the elements of _T_'s [[Sequence]] value. | ||
1. Let _len_ be the number of elements in _list_. | ||
</emu-alg> |
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 good if you could avoid duplicating the below section with Array.prototype.sort, since it's just the same, right?
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.
Most of it is the same, except with the handling of sparse arrays removed. We could share more here. What would be the best way to share more of this? Not sure how to handle it, since it isn't quite an abstract-op, very prose-y.
Merging this in so that we can continue to iterate on this in smaller chunks and merge it into the other and upcoming PRs. |
I'll rebase on this as well since you improved the ecmarkup pipeline |
Initial Record/Tuple standard library spec
NOTE: This is an early and incomplete draft. Comments welcome.