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

Should from() always return new objects? #341

Closed
Ms2ger opened this issue Feb 6, 2020 · 8 comments · Fixed by #1274
Closed

Should from() always return new objects? #341

Ms2ger opened this issue Feb 6, 2020 · 8 comments · Fixed by #1274
Assignees
Labels
spec-text Specification text involved
Milestone

Comments

@Ms2ger
Copy link
Collaborator

Ms2ger commented Feb 6, 2020

In #231 (comment), @ljharb says:

even if immutable and frozen, it still has identity (===, Object.is, Map/WeakMap keys, Set/WeakSet membership) and it's important to be able to easily create a new identity.

It seems like Temporal.Date.from(temporalDate) would be the most sensible way to support this requirement, more so than anything involving with().

I seem to recall that we discussed this before, but I can't find the discussion now.

@sffc
Copy link
Collaborator

sffc commented Feb 20, 2020

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 5, 2020

I support this change even more after discovering that you otherwise cannot balance an existing Duration by calling Temporal.Duration.from(existingDuration, { disambiguation: 'balance' }).

@ptomato
Copy link
Collaborator

ptomato commented Mar 6, 2020

I didn't notice before, but this is a duplicate of #232.

@gibson042
Copy link
Collaborator

#232 appears to have been resolved without an explicit decision on this point; I'm going to reopen it to avoid further duplication until that happens.

@gibson042 gibson042 reopened this Nov 16, 2020
@ptomato
Copy link
Collaborator

ptomato commented Nov 16, 2020

I'm fairly sure we have decided in some other context that from() always returns a new object (except for TimeZone and Calendar where it cannot), just not in the context of #232.

@gibson042
Copy link
Collaborator

I thought so too, but that's not the currently the case in the slides or (part of) the spec.

@ptomato
Copy link
Collaborator

ptomato commented Nov 16, 2020

I think the language in the slides is ambiguous and could be changed, but the spec text is correct as far as I can tell; ToTemporalFoo returns the same instance when used internally for coercion, but Temporal.Foo.from contains an extra step that clones the object if it is already of the correct type.

@ptomato
Copy link
Collaborator

ptomato commented Jan 13, 2021

I double-checked this, and there was indeed a discrepancy in the ZonedDateTime.from spec text. I've opened a pull request to address it.

@ptomato ptomato added the spec-text Specification text involved label Jan 13, 2021
@ptomato ptomato added this to the Stage 3 milestone Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants