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

Passing arguments vs. ignoring arguments #5

Open
js-choi opened this issue Mar 29, 2022 · 21 comments
Open

Passing arguments vs. ignoring arguments #5

js-choi opened this issue Mar 29, 2022 · 21 comments
Labels
question Further information is requested

Comments

@js-choi
Copy link
Collaborator

js-choi commented Mar 29, 2022

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:

  • Pass arguments to the first call and cache and return its result every time.
  • Ignore arguments to the first call and cache and return its result every time.
  • Pass arguments to the first call and return undefined every time.
  • Ignore arguments to the first call and return undefined every time.

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.

@js-choi js-choi added the question Further information is requested label Mar 29, 2022
@michaelficarra
Copy link
Member

michaelficarra commented Mar 29, 2022

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
Copy link
Collaborator Author

js-choi commented Mar 30, 2022

If I am not mistaken, @michaelficarra’s implementation matches the behavior preferred by @waldemarhorwat and @ljharb as expressed at plenary today. See also #2 (comment).

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
Copy link
Member

ljharb commented Mar 30, 2022

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
Copy link

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
Copy link
Member

ljharb commented Mar 30, 2022

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

@jridgewell
Copy link
Member

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
Copy link
Collaborator Author

js-choi commented Mar 31, 2022

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 (#2 (comment)), 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
Copy link
Member

ljharb commented Mar 31, 2022

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

@js-choi
Copy link
Collaborator Author

js-choi commented Mar 31, 2022

@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
Copy link
Member

ljharb commented Mar 31, 2022

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
Copy link
Member

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
Copy link
Collaborator Author

js-choi commented Mar 31, 2022

@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
Copy link
Member

ljharb commented Mar 31, 2022

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
Copy link
Collaborator Author

js-choi commented Mar 31, 2022

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 (#5 (comment)). 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
Copy link
Member

ljharb commented Mar 31, 2022

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

@js-choi
Copy link
Collaborator Author

js-choi commented Mar 31, 2022

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
Copy link

waldemarhorwat commented Apr 16, 2022

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
Copy link
Member

@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
Copy link
Collaborator Author

js-choi commented Jul 10, 2022

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 #2 (comment).

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
Copy link
Member

ljharb commented Jul 10, 2022

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
Copy link
Collaborator Author

js-choi commented Jul 10, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants