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

Symbols or strings for Temporal.Calendar and Temporal.TimeZone methods? #310

Closed
littledan opened this issue Jan 13, 2020 · 12 comments · Fixed by #487
Closed

Symbols or strings for Temporal.Calendar and Temporal.TimeZone methods? #310

littledan opened this issue Jan 13, 2020 · 12 comments · Fixed by #487
Labels
calendar Part of the effort for Temporal Calendar API has-consensus

Comments

@littledan
Copy link
Member

We discussed this a bit earlier, e.g., in #289 (comment) , but I wanted to open a separate issue to come to a conclusion on this one detail.

@sffc proposed using symbols stored as static properties, following the protocol proposal. I'd suggest using simple strings, as the protocols proposal has not been adopted, and strings are used for methods like this that already exist. I don't think we have to worry about namespace collisions here, which motivate some symbol use, and I think methods like Temporal.Calendar.prototype.plus are meaningful.

@sffc

This comment has been minimized.

@sffc
Copy link
Collaborator

sffc commented Jan 13, 2020

I don't have a strong opinion on string vs. symbol. If TC39 wants to eventually move in the direction of symbol-based protocols, then we should use symbols. But if TC39 doesn't intend to move forward with that proposal, then we should use strings.

@michaelficarra? Comments?

@littledan
Copy link
Member Author

littledan commented Jan 13, 2020

Currently, I don't think TC39 has decided on whether to move forward with the protocol proposal. I was hoping we could decouple these, and make Temporal work whether or not protocols eventually happen in the proposed form. I'm looking forward to @michaelficarra's comments.

@ljharb
Copy link
Member

ljharb commented Jan 13, 2020

The language has both String and Symbol based protocols, I don't think the first-class protovol proposal would force one or the other (since imo it needs to work with strings, too, to represent thenables/tostringables/valueofables/tojsonables)

@littledan
Copy link
Member Author

@ljharb I'm not sure if representing those things is a goal of the protocol proposal.

@ljharb
Copy link
Member

ljharb commented Jan 13, 2020

No, but it’s a requirement for advancement i have that ive made clear on that proposal repo.

@pipobscure
Copy link
Collaborator

I'm in favour of keeping methods as strings. While these are protocols as well, they have direct value being called directly by programmers as well. Symbol methods seem strange in that context. To my mind (and I might well be wrong) symbol-protocols are useful to allow implementation by random objects. I'm thinking of examples like iteration; random objects can become iterable by implementing a Symbol based protocol.

TimeZone & Calendar seem much more direct. Some random object couldn't just turn into a timezone. That just seems a bit unintuitive.

@ptomato ptomato added the calendar Part of the effort for Temporal Calendar API label Jan 22, 2020
@michaelficarra
Copy link
Member

@littledan

I'm not sure if representing those things is a goal of the protocol proposal.

The first-class protocols proposal includes requiring but not providing string-named properties as part of a protocol. See this slide from the most recent presentation to the committee.

@sffc @pipobscure

If TC39 wants to eventually move in the direction of symbol-based protocols, then we should use symbols. But if TC39 doesn't intend to move forward with that proposal, then we should use strings.

TC39 has already been using symbol-based protocols. This is in no way dependent on my proposal, the purpose of which is to represent these protocols as a value that you can refer to and pass around. If you've identified a protocol that developers will be using to intentionally coordinate with built-ins or with other developers, it should be symbol-based. This is especially the case when you're considering protocols with a method named something very generic like "plus" and objects that implement that protocol may need to implement other protocols.

@littledan
Copy link
Member Author

If you've identified a protocol that developers will be using to intentionally coordinate with built-ins or with other developers, it should be symbol-based.

We've decided not to do this with Promises and RegExp subclassing. Do you see these as mistakes not to be repeated?

objects that implement that protocol may need to implement other protocols.

It could be useful to think through cases where this occurs, to understand how serious the issue of overlap is. Can you think of a scenario like this?

@michaelficarra
Copy link
Member

@littledan

We've decided not to do this with Promises and RegExp subclassing. Do you see these as mistakes not to be repeated?

Absolutely. The use of a string-based protocol for Promises has lead to countless bugs. We've even recently discussed the issue arising from using dynamic import on a module that exports a then function within plenary. Off the top of my head, I can't think of any single bigger mistake in the language than the handling of then in Promises.

It could be useful to think through cases where this occurs, to understand how serious the issue of overlap is. Can you think of a scenario like this?

I'm not familiar enough with this particular design to comment on that. I was just providing you with background and a framework for you to use when making the decision. If the objects you expect to implement this protocol truly are single-purpose, a string-based protocol may be acceptable (though still probably not preferable).

@sffc
Copy link
Collaborator

sffc commented Feb 13, 2020

We discussed this at TC39. The notes from that discussion will be available at tc39/notes once approved. The general sentiment was that we should use strings here, because this is most likely the primary identity of a Calendar/TimeZone object, insofar as you aren't adding these calendar functions as you might add an iterator function to an existing type.

One interesting option that was suggested was to "hide" the string fields inside a single symbol, such that you could call something like myCalendar[Temporal.Calendar.fields].weekOfYear.

I think we should definitely move forward with the fields themselves being strings, which is also required for #291. The remaining open question then is whether we add a symbol to siphon the fields into another object, or whether we put them on the main object.

@ptomato
Copy link
Collaborator

ptomato commented Feb 20, 2020

Meeting Feb 20: Proceed with strings, without an extra symbol to encapsulate them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
calendar Part of the effort for Temporal Calendar API has-consensus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants