tc39 / proposal-function-once

A TC39 proposal for an Function.prototype.once method in the JavaScript language.
BSD 3-Clause "New" or "Revised" License
46 stars 3 forks source link

Passing arguments vs. ignoring arguments #5

Open js-choi opened 2 years ago

js-choi commented 2 years ago

If we cache the result of the first call (see #2), then subsequent calls’ arguments might be different and might not match those of the first call. Although this is what most userland libraries do today, @waldemarhorwat pointed out that this may be an antipattern. So our possibilities are:

At the plenary meeting today on 2022-03-29, most representatives generally called for not passing arguments and returning undefined every time (see #2).

We need to do research to see if there are any use cases for non-nullary “once” functions that are not event handlers. Non-nullary “once” event handlers may be better covered by devoted methods or options on the target, such as DOM EventTarget’s addEventListener’s { once: true } option or Node EventEmitter’s .once method. All of the real-world use cases I can find so far involve nullary callbacks.

michaelficarra commented 2 years ago

Here's my ideal implementation that wraps up all of my preferences:

const once = fn => {
    let completed = false, errored = false, storedReturn, storedError;
    return () => {
        if (!completed) {
            try {
                storedReturn = fn();
            } catch (e) {
                storedError = e;
                errored = true;
            }
            fn = void 0;
            completed = true;
        }
        if (errored) {
            throw new Error(storedError.message, { cause: storedError });
        } else {
            return storedReturn;
        }
    };
}

Written differently,

const once = fn => {
    let impl = () => {
        try {
            let storedReturn = fn();
            impl = () => storedReturn;
        } catch (e) {
            impl = () => { throw new Error(e.message, { cause: e }); };
        }
        return impl();
    };
    return () => impl();
}
  1. Ignores the arguments and this value given to the function returned by once. Does not pass them through to the function passed to once.
  2. The function returned by once will always either return the result of calling the function passed to once or throw an error whose cause is the error thrown by the function passed to once.
js-choi commented 2 years ago

If I am not mistaken, @michaelficarra’s implementation matches the behavior preferred by @waldemarhorwat and @ljharb as expressed at plenary today. See also https://github.com/js-choi/proposal-function-once/issues/2#issuecomment-1081942617.

I was wrong; I misread the code. @michaelficarra’s code caches the result and returns/throws the result every time, which @waldemarhorwat is uncomfortable about.

ljharb commented 2 years ago

Note that storedError might not be an Error object, if the function does throw null then that needs to be preserved too :-)

otherwise that works fine for me.

zloirock commented 2 years ago

I'm strictly for passing arguments. For example, the case of callbacks - event listeners. Sometime they should be called one time, but they depends on the passed event argument. Ignore of the argument for a such case is a mistake.

ljharb commented 2 years ago

Passing the argument but ignoring it on successive calls when it might be different seems like a mistake also, though.

jridgewell commented 2 years ago

Passing the argument but ignoring it on successive calls when it might be different seems like a mistake also, though.

It is a mistake blessed by precedent. Removing it will only make it more difficult to adopt the native feature over lodash's implementation.

js-choi commented 2 years ago

It is a good point that once-only event listeners often use their event arguments:

el.addEventListener('click', console.log, { once: true });

For what it’s worth, another option is for the new function to store references (weak references?) to its first call’s arguments (and this receiver and new.target).

Any subsequent call to the new function would check whether its arguments etc. match its first call’s. If they are identical, then the subsequent call is a no-op. If they do not match, the new function throws.

const fn = (function (x) { return }).once();
fn(1); // Prints 1.
fn(1); // Does nothing.
fn(0); // Throws.

This approach might be okay—if most subsequent calls on once functions use the same arguments. (And once-only event listeners should be removed from their event target after their first call, anyway.) And the approach would enforce function-call idempotence.

@ljharb: I know that you do not like throwing on every subsequent call (https://github.com/tc39-transfer/proposal-function-once/issues/2#issuecomment-1081947765), but perhaps throwing only on subsequent calls with different arguments might be better: allowing the first call to supply arguments while still preserving idempotence.


@jridgewell: It seems like you might agree that it would have been ideal if Lodash had different behavior in the first place, but that we are stuck with what developers are used to. I’ve opened an issue devoted to whether we should prioritize first principles or userland precedent first (#9).

ljharb commented 2 years ago

@js-choi that would be better, but how do you define "different arguments"?

js-choi commented 2 years ago

@ljharb: We would probably SameValue semantics, although SameValueZero may also be reasonable.

const fn = (function (x) { return }).once();
const o = {};
fn(o); // Prints 1.
fn(o); // Does nothing.
fn({}); // Throws.
const fn = (function (x) { return }).once();
fn(0); // Prints 1.
fn(0); // Does nothing.
fn(Object(0)); // Throws.
const fn = (function (x) { return }).once();
fn(0); // Prints 1.
fn(0); // Does nothing.
fn(-0); // Throws?
ljharb commented 2 years ago

Do two calls to the same event handler for the same event and element receive the same event object, or do they receive a distinct object that's conceptually equal?

jridgewell commented 2 years ago

it would have been ideal if Lodash had different behavior in the first place, but that we are stuck with what developers are used to

No, I believe lodash's behavior is correct, and explicitly write code that depends on its behavior. We could debate error throwing, but passing arguments and caching the first return value is exactly the right semantics.

Do two calls to the same event handler for the same event and element receive the same event object, or do they receive a distinct object that's conceptually equal?

They're distinct identities. Any memoization would defeat using fn.once() with as an event listener.

js-choi commented 2 years ago

@ljharb: Could you clarify what sort of situation you’re thinking of? I’m assuming you’re talking about event targets in the DOM. Two different clicks will create two different event objects. I’m not aware of any DOM events that may cause the event listener to be called more than once for the very same trigger at the very same time. (Although the DOM has { once: true } anyway…)

ljharb commented 2 years ago

yes, that's what i'm talking about. which means that throwing on "different" arguments would cause a onced event listener to always throw after the first call.

js-choi commented 2 years ago

Yes, that is right; there’s just no way around the dilemma:

If we want to pass the arguments, we either must either completely ignore them on subsequent calls (and possibly cause “misleading” behavior when calling with different arguments?) or throw on subsequent calls with different arguments (including nonidentical event objects).

@ljharb, you mentioned that using the first call’s arguments but then completely ignoring subsequent calls’ arguments might seem like a mistake (https://github.com/tc39-transfer/proposal-function-once/issues/5#issuecomment-1083072669). But, even with those misgivings, would you agree that that’s what we need once to do, in order for it to be useful with event targets?

(An aside: I already mentioned that DOM EventTargets already have { once: true }. Node’s EventEmitters also have once(). We may therefore want to exclude DOM EventTargets and Node EventEmitters from our decision making. All but one of the real-world examples in the explainer do not involve DOM EventTargets or Node EventEmitters. In general, event-based systems probably need special infrastructure to deal with one-off event handlers, anyway. Having said that…it still might useful for the first call to use its arguments.)

ljharb commented 2 years ago

I agree that since events already have a "once", we shouldn't be too concerned with them.

js-choi commented 2 years ago

Yes, and to continue pointing at the real-world examples in the explainer, we should look at what real-world scenarios other than EventTargets/EventEmitters would actually use their first calls’ arguments.

With the exception of the glob snippet, which uses a Node EventEmitter (and which arguably should be removed from the explainer), every one of those explainer real-world examples uses a nullary callback.

Therefore, research probably needs to be done: we need to search for real-world examples of once functions with unary or n-ary callbacks that actually use their arguments—and which aren’t already handled by DOM EventTargets or Node EventEmitters.

(Note: The real-world examples currently in the explainer were obtained by selecting some of the most-downloaded libraries that were dependent on various once-function libraries like onetime and lodash.once.)

waldemarhorwat commented 2 years ago

michaelficarra wrote:

Here's my ideal implementation that wraps up all of my preferences:

const once = fn => {
    let completed = false, errored = false, storedReturn, storedError;
    return () => {
        if (!completed) {
            try {
                storedReturn = fn();
            } catch (e) {
                storedError = e;
                errored = true;
            }
            fn = void 0;
            completed = true;
        }
        if (errored) {
            throw new Error(storedError.message, { cause: storedError });
        } else {
            return storedReturn;
        }
    };
}

Written differently,

const once = fn => {
    let impl = () => {
        try {
            let storedReturn = fn();
            impl = () => storedReturn;
        } catch (e) {
            impl = () => { throw new Error(e.message, { cause: e }); };
        }
        return impl();
    };
    return () => impl();
}

@michaelficarra: Did you notice that these two "ideal" implementations have observably different behavior? Which one did you intend?

(I prefer the second one)

michaelficarra commented 2 years ago

@waldemarhorwat No, I didn't. What's an example where the output/effects differ? I'm sure either impl would be fine with me.

js-choi commented 2 years ago

Ignoring the issue of what value to return (#2), I’d like to revisit caching the arguments of the first call and checking them on any subsequent call. This would ameliorate @waldemarhorwat’s pitfall in https://github.com/tc39/proposal-function-once/issues/2#issuecomment-1075776437.

function once (callback) {
  // This variable is undefined before callback is called.
  // After the first call, the variable is an array that caches the
  // this-receiver, new.target, and arguments of the first call.
  let previousCall;

  return function callOnce (...args) {
    const currentCall = [ this, new.target, ...args ];
    if (previousCall) {
      const callsAreIdentical = (
        currentCall.length === previousCall.length &&
        currentCall.every((a, i) => a === previousCall[i])
      );
      if (callsAreIdentical) {
        /* Return undefined or return a cached result; see issue #2. */;
      } else {
        throw new Error(
          `A once function was given different arguments than its first call.`,
        );
      }
    } else {
      previousCall = currentCall;
      callback.call(receiver, new.target, ...args);
      /* Return undefined or cached the result and return it; see issue #2. */;
    }
  };
}

In other words:

function f(x) {return x*x;}
const fOnce = f.once();
fOnce(4); // Doesn’t throw. Might return 16 or undefined; see issue #2.
fOnce(4); // Same.
fOnce(7); // Throws an error because zeroth argument is not 4.
ljharb commented 2 years ago

What semantics would you use for caching the arguments?

What if i call it with a mutable object, and then mutate the object later, and pass the same object, expecting different behavior?

js-choi commented 2 years ago

@ljharb: I was using === semantics. But you are right, I had forgotten about object mutability. Curses, that was the best chance I saw of passing, and not ignoring, arguments without pitfalls.