CanadaHonk / proposal-math-clamp

A TC39 proposal to add Math.clamp
https://canadahonk.github.io/proposal-math-clamp/
MIT License
36 stars 1 forks source link

Should min and max be optional? #9

Open CanadaHonk opened 1 day ago

CanadaHonk commented 1 day ago

Personally unsure currently. Options:

  1. Keep as is, min and max are optional.
  2. Make min (2 args) required, max optional.
  3. Make min and max (3 args) required.

See: https://github.com/tc39/how-we-work/blob/main/normative-conventions.md#when-required-arguments-are-missing-throw

theScottyJam commented 1 day ago

A few misc thoughts.

Math.clamp(n, 0)

I don't like this. This is basically a shorthand for clamping at a lower-bound, and it just feels wrong for this shorthand to only exist for lower bounds and not upper bounds. I'd rather this just isn't supported, and it gives a TypeError if you tried to do it. I also find it very unintuitive to read code like this.

Math.clamp(42) // => 42

This is just silly. ideally it wouldn't be allowed, but there's not really any harm in allowing it either, since there's no practical reason to ever see this kind of thing.

Math.clamp(n, 0, null);
Math.clamp(n, null, 360);

I find these forms to be useful. You can technically accomplish the same thing with infinity, but I think it's a (tiny) bit more expressive to use a nullish value here instead.

Math.clamp(n, 0, undefined);
Math.clamp(n, undefined, 360);

If using null causes you to fall back to a default parameter, then it makes even more sense for undefined to also do so, as that's how undefined is generally treated in functions.

However, I would find it weird if Math.clamp(n, 0, undefined); were allowed and Math.clamp(n, 0); was not, as most JavaScript functions don't make that kind of distinction.

It also feels really weird to write Math.clamp(n, 0, undefined), as my gut reaction when I see a function like that is to remove the last argument from the argument list, as it isn't doing anything - except doing so would hurt readability in this specific scenario, as discussed above.

So, in the end, it feels like I have this competing set of ideals where I'm going to have to give up something, but I don't know what I would want to give up. Perhaps my preference would be to make all parameters required and non-nullish, and if you want to omit a lower or upper bound, you have to use an infinity - but I don't really know.

CanadaHonk commented 1 day ago

I am preferring 3 personally and a few other delegates I spoke to hinted towards that option as well; there seems to be a precedent lately to be more strict with argument count. Possibly you could pass undefined/null explicitly but still require 3 arguments, and it could be then default to +-Infinity but undecided on that.

Richienb commented 1 day ago

you have to use an infinity

This would make it more cumbersome to use than userland...

It also feels really weird to write Math.clamp(n, 0, undefined), as my gut reaction when I see a function like that is to remove the last argument from the argument list, as it isn't doing anything - except doing so would hurt readability in this specific scenario, as discussed above.

We can still keep them optional, and in this case you would still use Math.clamp(n, 0, Infinity)

ljharb commented 1 day ago

fwiw TC39 will no longer be coercing things in new APIs, which means that null and undefined would both throw in this method. undefined would be allowed for an optional argument, of course.

CanadaHonk commented 1 day ago

Made all required in 513523edf172b71060b96249e90be8c7db997e9d

theScottyJam commented 11 hours ago

fwiw TC39 will no longer be coercing things in new APIs, which means that null and undefined would both throw in this method.

If it did coerce, I assume it would coerce to zero, correct?

In my examples, I was assuming that undefined and/or null weren't being coerced, but rather, they had special behavior tied to them to make them behave as shorthand for -Infinity or Infinity.

But I'm good with undefined/null throwing errors and requiring an explicit -Infinity/Infinity instead.