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

ZonedDateTime.prototype.startOfDay: Redundant given ZonedDateTime.prototype.withPlainTime can be called with zero arguments #3011

Open
anba opened this issue Oct 15, 2024 · 4 comments
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns

Comments

@anba
Copy link
Contributor

anba commented Oct 15, 2024

Calling ZonedDateTime.prototype.withPlainTime without any arguments is equivalent to calling ZonedDateTime.prototype.startOfDay, which makes ZonedDateTime.prototype.startOfDay kind of redundant.

I don't see startOfDay mentioned anywhere in the "size suggestion" threads, so I just want to make sure that this wasn't overlooked and it is intentional to have the separate startOfDay method for ZonedDateTime.

@justingrant
Copy link
Collaborator

Great catch! The difference was intended to be that startOfDay always returns the first instant of the day (which may not be 00:00 in the case of a forward DST transition, and should be the earlier 00:00 in the case of a backwards transition.

But (correct me if I'm wrong) that is the default behavior of the compatible disambiguation option that's used in ZDT.p.withPlainTime().

@ptomato I'm not sure it's worth cracking normative changes open again for this, but it does indeed seem like we could safely remove this method, and probably would have removed it had any of us caught it before. Maybe keep this in our back pocket in case another normative change is discovered? Or would it be better to be proactive so that unflagged implementations aren't shipped with a method we might remove? Or just don't rock the boat now at all?

@khawarizmus
Copy link
Contributor

Wouldn't that method be useful for lunar calendars as explained in this issue js-temporal/proposal-temporal-v2#30 ?

@ptomato
Copy link
Collaborator

ptomato commented Oct 15, 2024

Great catch! The difference was intended to be that startOfDay always returns the first instant of the day (which may not be 00:00 in the case of a forward DST transition, and should be the earlier 00:00 in the case of a backwards transition.

But (correct me if I'm wrong) that is the default behavior of the compatible disambiguation option that's used in ZDT.p.withPlainTime().

That used to be the behaviour of withPlainTime with no arguments, until our very last normative change #2918. Now they are equivalent. I think that's also why we didn't consider startOfDay for removal. At the time, it didn't do the same thing as withPlainTime.

I'm not sure it's worth cracking normative changes open again for this, but it does indeed seem like we could safely remove this method, and probably would have removed it had any of us caught it before. Maybe keep this in our back pocket in case another normative change is discovered? Or would it be better to be proactive so that unflagged implementations aren't shipped with a method we might remove? Or just don't rock the boat now at all?

I think we need to continue considering the proposal frozen, unless we explicitly get feedback from implementations that it's still too large to ship. And if we did get that feedback, we'd need to consider more drastic changes— it seems really unlikely that this one method would make the crucial difference.

Wouldn't that method be useful for lunar calendars as explained in this issue js-temporal/proposal-temporal-v2#30 ?

Yes, but if we had to remove it, we'd have to add it again if adding support for days starting at sunrise/sunset.

@ptomato ptomato added the size-suggestion Suggestions to shrink size of Temporal to address implementer concerns label Oct 15, 2024
@justingrant
Copy link
Collaborator

Wouldn't that method be useful for lunar calendars as explained in this issue js-temporal/proposal-temporal-v2#30 ?

Yes, but if we had to remove it, we'd have to add it again if adding support for days starting at sunrise/sunset.

Yep. We'd also have to have it accept lat/long/elevation options, and the default (no option) behavior would still return midnight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns
Projects
None yet
Development

No branches or pull requests

4 participants