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

Fix subclassing support and new object creation in Temporal.*.from #232

Closed
littledan opened this issue Oct 30, 2019 · 7 comments
Closed
Assignees
Labels
behavior Relating to behavior defined in the proposal non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@littledan
Copy link
Member

A couple issues with these from methods:

  • In the spec text (but not the polyfill), the this value seems to be ignored. We should be returning an instance of this. However, the polyfill does not quite do this the way I imagined, as explained below.
  • If we want to be analogous to existing JS from methods, this is logically a way to construct a new object that's based on an existing one, and not a cast; it will always create a new instance (just like Array.from does applied to an Array). However, the logic in the polyfill and spec text will not create a new instance if it's provided an existing instance of the same class.

I'd suggest refactoring the implementation of from and casting in general as follows:

  • Cut out the core of it, into a new abstract algorithm, which is to Get the appropriate properties from a *-like object, or get the internal slots if available, and put them in a record
  • The from method (when applied on something that's not a String) will call this new abstract algorithm, and then construct this with the result
  • The cast algorithm will check if something's already the right Temporal type, if so, return that, otherwise call the internals of the from method (with the original constructor as the new.target). [I don't mean to literally invoke Construct but rather use these algorithm steps, deduplicated into an abstract algorithm.]
@ryzokuken ryzokuken self-assigned this Nov 1, 2019
@ryzokuken ryzokuken added behavior Relating to behavior defined in the proposal non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Nov 1, 2019
@ryzokuken ryzokuken added this to the November Meeting milestone Nov 1, 2019
@ryzokuken
Copy link
Member

I agree with the suggestions by @littledan and will be fixing this alongside #230 in a new PR.

In the meantime, @Ms2ger @pipobscure @ljharb I'd love to hear your thoughts on this.

@ljharb
Copy link
Member

ljharb commented Nov 1, 2019

I agree with both of the two bullet points in the OP.

@ptomato
Copy link
Collaborator

ptomato commented Mar 6, 2020

Relevant comment from the duplicate:

sffc>

from() should delegate to, e.g., the dateFromFields() method on the calendar. So, if we add a requirement that from() always returns a new object, we need to decide to either (1) make that a recommendation for Calendar implementers, or (2) enforce it by cloning the object in the from() spec.

@ptomato
Copy link
Collaborator

ptomato commented Mar 16, 2020

As per #231 and #237, we're only doing casting in with(), which is covered by ToPartialDate, etc., and plus() and minus(), which are covered by ToLimitedDuration(). We also decided somewhere else (can't remember where) that from() converts any non-Object to a String. So the plan described in the top comment becomes a bit simpler, we don't actually need a casting method.

@ptomato ptomato self-assigned this Mar 18, 2020
@ptomato
Copy link
Collaborator

ptomato commented Mar 18, 2020

I have a work in progress for this, but it's going to collide with the test262 work that @Ms2ger is doing, so I'll put it on pause for now.

ptomato added a commit that referenced this issue Mar 18, 2020
Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
@ptomato
Copy link
Collaborator

ptomato commented Mar 18, 2020

@Ms2ger My work in progress is at https://github.com/tc39/proposal-temporal/tree/232-from-cloning, feel free to reorganize the commits however you like.

@ptomato ptomato removed their assignment Mar 18, 2020
Ms2ger pushed a commit that referenced this issue Mar 19, 2020
Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger pushed a commit that referenced this issue Mar 20, 2020
Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger pushed a commit that referenced this issue Mar 25, 2020
Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger pushed a commit that referenced this issue Mar 26, 2020
Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger pushed a commit that referenced this issue Mar 26, 2020
Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger pushed a commit that referenced this issue Mar 26, 2020
Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger added a commit that referenced this issue Mar 26, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 1, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 1, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 1, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 1, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 1, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Effectively, this makes Temporal.Foo.from(aFoo) clone the object instead
of returning the same object.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 1, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 2, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
@ptomato ptomato removed this from the November Meeting milestone Apr 3, 2020
Ms2ger added a commit that referenced this issue Apr 3, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
Ms2ger added a commit that referenced this issue Apr 6, 2020
Absolute and TimeZone cannot take property bags, so they are different
from the rest, but in general the idea is to call ToX(arg) if Type(arg)
is Object, and XFromString(ToString(arg)) if not.

Reorganize from() to clone the object

Each type's from() method now calls a new abstract operation,
ToFooRecord, which takes a property bag (which can be the actual type)
and returns a Record with the appropriate slots.

If the argument isn't an object, then from() calls ParseFooString
directly. We delete the FooFromString operations, and in most cases the
ToFoo operations unless there is a method somewhere in the API that
actually needs the casting behaviour (including returning the original
object if it's of the correct type.)

Reorganizes the abstract operations in the polyfill to match the
abstract operations in the spec a bit more.

Closes: #232.
@ptomato
Copy link
Collaborator

ptomato commented May 12, 2020

I think this was fixed by #502 some while ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

4 participants