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

size-suggestion: Consider removing subtract in favor of add #2844

Closed
justingrant opened this issue May 20, 2024 · 20 comments
Closed

size-suggestion: Consider removing subtract in favor of add #2844

justingrant opened this issue May 20, 2024 · 20 comments
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns

Comments

@justingrant
Copy link
Collaborator

We could remove all 7 subtract prototype methods from Duration, Instant, PlainDate, PlainDateTime, PlainTime, PlainYearMonth, and ZonedDateTime. Users would use add with negative values instead.

For duration literals, negation would mean using negative numbers. With Duration parameters, the .negated() method could be used.

This issue is adapted from suggestions made by @FrankYFTang to help reduce the size of Temporal to address concerns from implementers (especially Android and Apple Watch) that Temporal's install size and/or in-memory size is too large.

@justingrant justingrant added the size-suggestion Suggestions to shrink size of Temporal to address implementer concerns label May 20, 2024
@gibson042
Copy link
Collaborator

Removing subtract would be a substantial ergonomic loss; it is much easier to read and comprehend a.subtract(b) than a.add(b.negated()) (let alone a.add(Temporal.Duration.from(b).negated())).

@FrankYFTang
Copy link
Contributor

My origional suggestion is in https://docs.google.com/document/d/16Rd43XB8z6iLC2c9AfF--unDFg_t476tI1PFe7PbbcM
I didn't suggest 'negated()' , I only suggest fold the current subtract funcitonality into add . I agree there will be ergonomic impact I will be a trade off between ergonomic vs API simplicity without cutting out functionality.

@ptomato
Copy link
Collaborator

ptomato commented May 20, 2024

Agreed with Richard on this one, add and subtract are basic operations and they need to be at developers' fingertips.

@sffc
Copy link
Collaborator

sffc commented May 20, 2024

I find a.add(b.negated()) and a.subtract(b) to be similar in clarity. My initial reaction is that I disagree about this being a big ergonomic loss. It is also something really easy to add in a follow-on proposal if it turns out to be a pain point.

@justingrant
Copy link
Collaborator Author

My position matches Shane's. The workaround is intuitive and it'd be easy to add this in a follow-up proposal later.

@ptomato
Copy link
Collaborator

ptomato commented May 23, 2024

After talking to some users of Temporal this week I'm starting to come around on this point. Still would prefer not to do this, though.

@justingrant
Copy link
Collaborator Author

justingrant commented May 24, 2024

Meeting 2024-05-23: At the upcoming June 2024 TC39 plenary meeting, we'll propose removing the 7 subtract methods.

We think that this is OK because users can negate the duration and use add. And subtract could be easily added in a follow-up proposal. We'll make sure to document this behavior so that users know what to do.

@ptomato
Copy link
Collaborator

ptomato commented May 27, 2024

Removal of subtract can be seen here. It includes changes to the docs site, but I'd welcome help to make those better.

@FrankYFTang
Copy link
Contributor

Removal of subtract can be seen here. It includes changes to the docs site, but I'd welcome help to make those better.

thanks

Maybe also remove the "OrSubtractDurationFrom" part from the long AO name?

@nnmrts
Copy link

nnmrts commented Jun 2, 2024

After talking to some users of Temporal this week I'm starting to come around on this point. Still would prefer not to do this, though.

@ptomato Can you give insight into why you've changed your mind? Which user of Temporal told you removing subtract is fine for them? 😅

I realize I don't have any "say" in this matter and that I'm a bit late to the party, but I find the size-suggestions issues somewhat absurd, particularly this one. While it might sound harsh, a.add(b.negated()) and a.subtract(b) are objectively NOT "similar in clarity". I've been using Temporal through polyfills (and more recently through Deno/V8) for years now and have never used negated. I'm aware of its existence, but I would only use it if I needed the negated version of a duration for immediate use or display. Maybe I’m not great at math, but my brain can process the meaning of

$x - y$

much faster faster than

$x + -y$

and I hope I'm not the only one. If I'm not, that alone should justify keeping subtract. When considering the concept of time and trying to articulate it in plain English, it becomes even harder for me to grasp.

Assuming

const x = Temporal.PlainDate.from("2024-06-02");
const y = Temporal.Duration.from({days: 5});

compare

x.subtract(y)

from 2024-06-02, go 5 days back, making it 2024-05-28

to

x.add(y.negated())

first convert 5 days to 5 negative days (whatever that means in the real world), then add these 5 negative days to 2024-06-02, which is going back 5 days to 2024-05-28

Some statements from implementers also seem inconsistent. If the goal of removing these ergonomic features is to reduce size and complexity due to constraints of certain devices/systems, why would we expect them to be added later? If "it is also something really easy to add in a follow-on proposal", why remove it now? It seems more complicated to remove these features at this stage of the proposal, especially after so many users of Temporal polyfills/implementations have become accustomed to them. Not having subtract when you have add would feel like another frustrating but typical "surprise" in JS.

Concrete metrics or benchmarks from the implementers would be very helpful to understand the actual problem with keeping simple operations like subtract in the proposal. Just citing "function count" isn’t sufficient for me, sorry. Insights into how and when these removals might be revisited and readded would also be nice.

Disclaimer: I'm not a native English speaker, so my writing might come across as more impolite than I intend.

@FrankYFTang
Copy link
Contributor

you do not need to do that. Just put the - in front of the 5 is better.

const x = Temporal.PlainDate.from("2024-06-02");
const y = Temporal.Duration.from({days: -5});
x.add(y)

@FrankYFTang
Copy link
Contributor

If we add subtraction, should we add multiple or division to Duration too?
What if I want to add three time the duration of y to x or subtract half of the duration of y from x, do we also need to support

x.add(y.multiply(3));
x.subtract(y.divid(2));

then?
we can go on and on and find a lot of "useful usecases" if math is an important part of Temporal. But I don't think those are essential now, right? I am not supporting to add those, just the same reason I do not think we should add subtract now.

@justingrant
Copy link
Collaborator Author

Concrete metrics or benchmarks from the implementers would be very helpful to understand the actual problem with keeping simple operations like subtract in the proposal. Just citing "function count" isn’t sufficient for me, sorry. Insights into how and when these removals might be revisited and readded would also be nice.

The context is that engines, especially V8 which powers Chrome and Node, incur a fairly large cost (relative to the overall size of V8) in both install size and memory consumption for each function in a built-in object. FWIW, this was also a suprise to the champions of the Temporal proposal. We didn't anticipate that adding functions would be such a challenge for JS engines.

In a desktop browser rendering a simple web page, this isn't an issue. But there are cases where even a few tens of KB increase matters:

  • On lower-end Android devices, space (both persistent storage and RAM) is much more limited, but JS needs to be able to run anywhere, even low-end devices. Note that Apple's JS engine JavaScriptCore has similar challenges on Apple Watch devices.
  • When there are many <iframe> elements on a page (as is sadly common in a lot of adverising-heavy websites) then each iframe gets a separate copy of JS builtin objects, which chews up RAM.

For this reason, Google (specifically the Android team) limits the size increase per year of browser components like V8. Apple and Mozilla had similar concerns.

Thankfully, this gets better over time thanks to Moore's law. The low-end devices of 2 years from now will have a lot more storage, so this allows V8 to gradually get larger over time.

So in the meantime, we're planning to strip out lower-priority parts of the Temporal proposal. These can be easily added back in the future if there is sufficient demand from the community. Also, ergonomic helpers like subtract are trivially polyfillable, so for the large % of JS code that's transpiled, whenever a new proposal is approved it will be easy to use it, even if native implementations haven't added it yet.

Insights into how and when these removals might be revisited and readded would also be nice.

You can learn more about how ECMAScript is extended here: https://github.com/tc39/proposals

@nnmrts
Copy link

nnmrts commented Jun 5, 2024

@FrankYFTang

you do not need to do that. Just put the - in front of the 5 is better.

Which is still confusing though. Again, maybe it's "just me", but to me negative durations make no sense in the real world. For me and I think for most people, durations are always a positive span of time, that you can add to or subtract from a point in time to get to another point in time. Even ISO 8601-2:2019, which is a relatively new extension to the standard, doesn't specify if adding a negative duration to a date is the same as subtracting it (and how to handle that), it only specifies that adding negative durations to other durations is the same as subtraction - maybe because there could be differences?

If we add subtraction, should we add multiple or division to Duration too? What if I want to add three time the duration of y to x or subtract half of the duration of y from x, do we also need to support
...
then? we can go on and on and find a lot of "useful usecases" if math is an important part of Temporal. But I don't think those are essential now, right? I am not supporting to add those, just the same reason I do not think we should add subtract now.

That's just a slippery slope argument. No one said anything about adding a whole math suite to Temporal. But I find it weird you think adding the "opposite" of an operation like add is not essential. Also, multiplication and division aren't even in the same date-duration relationship as add and subtract. You can't really multiply or divide durations off of dates. That immediately makes those two objectively less useful. This proposal is at stage 3 and AFAIK never included them, whereas subtract was a part of it for a long time. So please don't frame this is as "adding" subtraction, this issue is about removing it.

@justingrant

I see, thanks for the insight. I am still astonished how subtract could possibly be "too much", but I guess in combination with other "unimportant" features it is...

Here's also an informative issue thread about code size and memory impact of Temporal, in case anyone else is confused by the motivation behind all of this and wants to see actual metrics (however accurate they are): #2786

You can learn more about how ECMAScript is extended here: tc39/proposals

Where do you think I came from years ago before discovering this proposal? 😅

I rather meant if there is any effort to not lose years of work from this proposal, maybe by splitting all removed spec directly to a new proposal, or by advertising a Temporal "extensions" polyfill in the future, or even by investigating in how implementers could still add these removed featured behind some "experimental" flag, either in runtime or when building. But that's probably something ECMAscript shouldn't define.

I remember there was some temporal extensions proposal out there, but I can't remember its name and couldn't find it.

EDIT: There it is: https://github.com/js-temporal/proposal-temporal-v2

@sffc
Copy link
Collaborator

sffc commented Jun 5, 2024

Temporal supports negative durations. #782

.add(duration) looks like it always goes into the future, and .subtract(duration) looks like it always goes into the past. However, because of negative durations, both functions could go in the other direction.

By having one way to do things, we make code that is less error-prone, and we leverage the opportunity to teach developers about negative durations, why they exist, how to use them.

@nnmrts
Copy link

nnmrts commented Jun 5, 2024

@sffc

I know that Temporal supports negative durations. Why ISO 8601-2:2019 specifies and Temporal supports it, I don't know though. Why do they exist? My only guess is that when calculating with dynamic durations, it's better to introduce the concept of negative durations than just throwing an error. Which is fine and nice to have but it doesn't make the concept itself more intuitive and therefore, at least for me, invalid as an argument to remove the more intuitive subtract.

.add(duration) looks like it always goes into the future, and .subtract(duration) looks like it always goes into the past. However, because of negative durations, both functions could go in the other direction.

By having one way to do things, we make code that is less error-prone, and we leverage the opportunity to teach developers about negative durations, why they exist, how to use them.

So, you too think that even if there were no code size or memory usage concerns, a subtract method would make this proposal worse? Because I feel like this is at least part of why this issue exists - people don't see the ergonomics of subtract and/or actually think it shouldn't have been there in the first place. From that perspective, it's much more understandable why one would see subtract as something that can just be dropped from the proposal. I disagree with that perspective a lot, but let's not go into why I think removing subtract will actually lead to more "error-prone" code and frustration.

I feel like it's too late for that anyway and to be fully honest I agree with @justingrant that "ergonomic helpers like subtract are trivially polyfillable". So to me it's not even worth it to argue over this more than I already did - I'll just include a temporal-ergonomics module every time I deal with Temporal in the future. But it will just be yet another quirk of JS that I still believe could have been avoided and that people will make fun of. At the very least, I think it's worth discussing further instead of just accepting that fact. I hope whatever happens at https://github.com/tc39/agendas/blob/main/2024/06.md addresses my concerns.

@ptomato
Copy link
Collaborator

ptomato commented Jun 7, 2024

@nnmrts Sure. I can answer the parts of your question that Justin didn't already answer.

Can you give insight into why you've changed your mind? Which user of Temporal told you removing subtract is fine for them? 😅

Of course any individual conversations are just anecdotal evidence, but yes, I did hear from some Temporal users that they wouldn't be particularly fussed by the lack of subtract().

The main reason I changed my mind is because its functionality is still present, it is trivially polyfillable, and could be re-added in the future in a web-compatible way in a small, simple, self-contained TC39 proposal.

We discussed removing a number of methods that fit these criteria. Of those, I think subtract is the biggest ergonomic loss. However, given the situation we are dealing with, namely skepticism from the people who actually have to deliver the proposal to users, I came around to the idea of letting these removals be future improvements which would be driven based on community demand (which, to an extent, is better than us predicting that an API will be widely used or not) and each considered individually as a tradeoff between ergonomics and implementation costs.

I get what you said about negative durations and that's important feedback, thanks. Personally, I've had no trouble considering negative durations just like negative numbers. So while I had a hunch that a.add(b.negated()) is harder to come up with as a workaround than a + -b would be with numbers, that helps to explain why.

About the general status of this issue. Certainly it's hard to articulate the range of opinions in the room while also trying to move forward a consensus position, but I think you'll have a more accurate understanding of the situation if you adjust this assumption:

So, you too think that even if there were no code size or memory usage concerns, a subtract method would make this proposal worse? Because I feel like this is at least part of why this issue exists - people don't see the ergonomics of subtract and/or actually think it shouldn't have been there in the first place.

Yes you heard this from Frank above, but I don't think it's accurate to assume it reflects a widely held view. I certainly don't think that the proposal is better without subtract and I don't believe that anyone in the room was happy to be rid of it when we discussed removing it. It existed for a good reason and I actually do expect that if anyone mounts an effort in TC39 to re-add any of these methods once Temporal is shipped, it'll be for subtract.

@FrankYFTang
Copy link
Contributor

FrankYFTang commented Jun 7, 2024

@nnmrts Currently, Temporal defines the following 7 subtract

3.3.24 Temporal.PlainDate.prototype.subtract ( temporalDurationLike [ , options ] )
4.3.10 Temporal.PlainTime.prototype.subtract ( temporalDurationLike )
5.3.30 Temporal.PlainDateTime.prototype.subtract ( temporalDurationLike [ , options ] )
6.3.39 Temporal.ZonedDateTime.prototype.subtract ( temporalDurationLike [ , options ] )
7.3.19 Temporal.Duration.prototype.subtract ( other [ , options ] )
8.3.8 Temporal.Instant.prototype.subtract ( temporalDurationLike )
9.3.15 Temporal.PlainYearMonth.prototype.subtract ( temporalDurationLike [ , options ] )

Do you think all of them are useful and important and you will use all 7 of them?

@nnmrts
Copy link

nnmrts commented Jun 7, 2024

@FrankYFTang Yes, all of them. For example, I used Temporal.PlainYearMonth.prototype.subtract from temporal-polyfill in a real-world project at my job months ago and ran into a nasty bug (leaking my work account here 😝): fullcalendar/temporal-polyfill#21

Ironically doing .add({day: -1}) would've saved me in that specific case, but not because it's more intuitive but because there was a bug in the polyfill. 😆 Nevermind, neither approach would've avoided that bug, as it was related to going forward vs. back in time and not add vs. subtract.

Here are some of the uses of it in our code base of that project, my colleague and I wrote:

Bildschirmfoto 2024-06-07 um 23 14 40

Bildschirmfoto 2024-06-07 um 23 16 36

As you can see, we even implemented our own PlainYearQuarter unit there because it was required that the interface showed stats in yearly, quarterly and monthly ranges and it was so much easier to work with doing it like that, especially when we needed to convert between months and quarters. I also developed a fairly complex date range picker with quarter support from scratch just for this project and would've gone insane without both our custom class and the subtract method.

But yeah, just in those screenshots above you can observe:

  • PlainDate.subtract
  • PlainDateTime.subtract
  • PlainYearMonth.subtract

and using our own quarter class of course:

  • PlainYearQuarter.subtract

@ptomato
Copy link
Collaborator

ptomato commented Jun 13, 2024

This did not reach consensus at the TC39 meeting of 2024-06-12.

@ptomato ptomato closed this as completed Jun 13, 2024
@ptomato ptomato closed this as not planned Won't fix, can't repro, duplicate, stale Jun 13, 2024
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

6 participants