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

Subsequent calls: cache, undefined, throw, or both #2

Open
js-choi opened this issue Mar 18, 2022 · 27 comments
Open

Subsequent calls: cache, undefined, throw, or both #2

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

Comments

@js-choi
Copy link
Collaborator

js-choi commented Mar 18, 2022

Let’s say we define:

function f (x) { return 6; }

We have four options with once.

Option 0: Cache and return cached result

Cache and return the value of the first call from any subsequent calls. This may cause the created function to behave more predictably.

const fOnce = f.once();
fOnce(); // Returns 6.
fOnce(); // Returns 6.

Sync errors thrown by the first call may either be cached and thrown by every subsequent call…or thrown only once.

We could pass arguments from the created function’s first call to its callback—or we could ignore arguments. There is an issue devoted to this: #5.

Option 1: Always return undefined

Return undefined from any subsequent calls. The created function’s return value may become less predictable, but it does let the developer more easily detect when a call is not the first call.

const fOnce = f.once();
fOnce(); // Returns 6.
fOnce(); // Returns undefined.

Sync errors thrown by the first call would be thrown only once.

Option 2: Always throw on subsequent calls

Throw an error after any subsequent calls. I would personally not expect this as default behavior.

const fOnce = f.once();
fOnce(); // Returns 6.
fOnce(); // Throws an error.

Sync errors thrown by the first call would be thrown only once.

Option 3: Multiple functions

Split off the Throw option into its own “strictOnce” function. Several existing libraries offer this. I’m neutral about it.

const fOnce = f.once();
fOnce(); // Returns 6.
fOnce(); // Returns 6.

const fStrictOnce = f.strictOnce();
fStrictOnce(); // Returns 6.
fStrictOnce(); // Throws an error.

Sync errors thrown by the first call may either be cached and thrown by every subsequent call…or thrown only once.

@js-choi js-choi added the question Further information is requested label Mar 18, 2022
@bakkot
Copy link

bakkot commented Mar 18, 2022

I think caching is what everything in the ecosystem does? (Underscore/lodash, at least.) Probably best to match that behavior.

For other behaviors, if we think they are important enough to support (which I am not offering an opinion on), I think we could reasonably have an argument, e.g. fn.once({ throwOnSecondCall: true }).

@zloirock
Copy link

SugarJS and some other libraries already provide Function.prototype.once and the behaviour different from the cache could break them.

@Jamesernator
Copy link

Jamesernator commented Mar 18, 2022

For other behaviors, if we think they are important enough to support (which I am not offering an opinion on), I think we could reasonably have an argument, e.g. fn.once({ throwOnSecondCall: true }).

I think this would be useful, alternatively we could even provide a function to let users decide i.e.:

fn.once({
    subsequentCalls: () => {
        console.warning("Resource has already been cleaned up, consider releasing the object earlier");
    },
});

fn2.once({
    subsequentCalls: () => {
        throw new Error("value has already been extracted and can no longer be consumed");
    },
});

fn3.once({
    subsequentCalls: () => {
        // Value not available so return null
        return null;
    },
});

fn4.once({
    // Maybe even allow for caching first return for later use
    cacheFirstReturnValue: true,
    subsequentCalls: (fd) => {
        throw new Error(`File descriptor ${ fd } is now stale`);
    },
});

If we did this, the only behaviour we would need to specify is what is the default for "subsequentCalls".

@hax
Copy link
Member

hax commented Mar 19, 2022

Allow specifying function value for "subsequentCalls" seems too powerful. Simple argument like fn.once({ throwOnSecondCall: true }) or simpler fn.once('throwOnSecondCall') seems enough.

@Jamesernator
Copy link

Allow specifying function value for "subsequentCalls" seems too powerful. Simple argument like fn.once({ throwOnSecondCall: true }) or simpler fn.once('throwOnSecondCall') seems enough.

The reason it's more useful to have a function, is you can't give any useful info about "why" calling more than once is an error. i.e. The examples above would allow you to customize the error to indicate why.

@hax
Copy link
Member

hax commented Mar 19, 2022

I just realize this issue is much tough than I thought.

I agree with @bakkot, Option 0 (cache) behavior seems the best match most ecosystem lib. And I don't like silent failure like Option 1 (undefined), but after checking real-world examples, it seems the most usages of once is triggering side-effect only once, the best match for such case might be once(): boolean which denote whether it triggered the side-effect. Option 2 (throw) might also work because most usages are async.

I also notice that there are issues of throwing. For Option 0/1, does the throwing also be cached? For Option 2, how to differentiate the reason of throwing?

@js-choi
Copy link
Collaborator Author

js-choi commented Mar 19, 2022

Yes, the error-caching question is a good one. We need to investigate what current libraries do here.

@waldemarhorwat
Copy link

Caching functions might be ok on functions that don't take any arguments but is inappropriate on functions that do take them.

function f(x) {return x*x;}

const fOnce = f.once();

if (...) {
  a = fOnce(4); // Returns 16
}
b = fOnce(7); // Returns either 16 or 49?

If you wanted b to get the value 16, this would be better and simpler written as:

function f(x) {return x*x;}

const f4 = f(4);

a = f4;
b = f4;

Another problem is that caching will deadlock or fail if the body of f recurses into fOnce.

@js-choi
Copy link
Collaborator Author

js-choi commented Mar 23, 2022

@waldemarhorwat: It does look like at least one once implementation (lodash.once; see also lodash.before) releases its reference to the original callback after calling it for the first time. Matching this behavior will probably be uncontroversial. However, lodash.once does cache the callback’s result for future calls.

See also #3.

@js-choi
Copy link
Collaborator Author

js-choi commented Mar 29, 2022

At the plenary meeting today, several representatives (@waldemarhorwat, @ljharb) seemed to support returning undefined every time, due to #5. (Errors from first call would throw only on the first call and not on subsequent calls—essentially being a once-only side effect.)

The argument is that #5 makes caching the first result into a pitfall and an antipattern, and that once should be used to guarantee that side effects are idempotent, rather than memoizing results. We can try to make a memoize helper function separately. Dissenting were @jridgewell and @bnb, who prefer matching ecosystem precedent and caching the first result. @michaelficarra seems to be fine with returning undefined but seems to prefer caching and returning the first result but ignoring arguments (#5).

This would also probably necessitate that this be renamed to Function.once (#1, #4).

@ljharb
Copy link
Member

ljharb commented Mar 29, 2022

I strongly support option 1.

Option 2 sucks because you want to be able to pass your onced function elsewhere, and you can't control what arguments it's called with.

Option 0 is indeed the precedent, but I would be very surprised if the wider ecosystem was even aware that once commonly memoizes - and it's a footgun to pass different arguments to it and get the same result.

Option 3 seems not particularly valuable.

If we want a memoize helper, I think we should indeed add one separately.

@js-choi
Copy link
Collaborator Author

js-choi commented Mar 29, 2022

(For what it’s worth, I am willing to champion a separate, configurable memoize helper function.)

@keithamus
Copy link
Member

From my comment which links to 9 different in-the-wild implementations: #4 (comment)

name returns original subsequent returns?
bit.js ✔️ undefined
aroma ✔️ undefined
@v4fire/functools ✔️ Cached*
ignotifier ✔️ undefined
kranium ✔️ false
fanta ✔️ Cached*
ofio ✔️ Cached*
PixieEngine/Cornerstone ✔️ Cached*
spolu/pipes ✔️ undefined

* Return values marked as "Cached" disregard arguments

Of note is the kranium utility which returns false, but I'm not entirely sure how intentional that is, or if it just does so for brevity in the source:

return function(){ 
    return ++i <= 1 && fn.apply(ctx || fn, slice.call(arguments)); 
}; 

@zloirock
Copy link

I'm strictly for the cached result.

@zloirock
Copy link

zloirock commented Mar 29, 2022

@keithamus lodash / underscore - cached, SugarJS - cached, once - cached, etc. - at least, the most that I know.

This was referenced Mar 30, 2022
@Ginden
Copy link

Ginden commented Mar 30, 2022

Option 0 is indeed the precedent, but I would be very surprised if the wider ecosystem was even aware that once commonly memoizes - and it's a footgun to pass different arguments to it and get the same result.

My code depends on this behavior. Moreover, it's explicitly written in eg. Lodash documentation. https://lodash.com/docs/4.17.15#once

@ljharb
Copy link
Member

ljharb commented Mar 30, 2022

It's in the prose, but in none of the code examples (same for underscore).

I'm sure there are a number of people that do use it this way - I'm just confident it's a very very small number.

@zloirock
Copy link

zloirock commented Mar 30, 2022

I'm just confident it's a very very small number.

I'm confident it's a very very big number. For example, the cached result in this case is a kind of the singleton software design pattern. It's used in most popular libraries like lodash. Personally my code depends on this behavior.

@ljharb
Copy link
Member

ljharb commented Mar 30, 2022

Are there any use cases y’all are aware of that take arguments? Or are you only using once as a zero-arg memoize function?

@zloirock
Copy link

zloirock commented Mar 30, 2022

For example, callbacks - event listeners? Some time they should be called one time, but they depends on passed event argument. Ignore of the argument for a such case is a mistake. However, it's another issue - #5.

@Ginden
Copy link

Ginden commented Mar 30, 2022

In my use cases it was always zero-arg memoize.

@jridgewell
Copy link
Member

I think that we should be matching lodash's cache and arg passing behavior exactly, and I have written code that depends on it.

@js-choi
Copy link
Collaborator Author

js-choi commented Mar 30, 2022

For what it’s worth, I have also created a proposal-function-memo, which might cover singleton use cases, and which I plan to present to a future plenary. It’s very unspecified right now, being at Stage 0.

@ljharb
Copy link
Member

ljharb commented Mar 30, 2022

I think we should always start from first principles, and consider userland precedent second to that.

@ckknight
Copy link

ckknight commented Apr 4, 2022

If caching is on the table, it might be prudent to explicitly ignore arguments, in order to guarantee same behavior. If arguments are allowed, the caching approach is either going to be wrong or correct-by-coincidence.

Background: I've used 0-arg cached functions quite a few times in our codebase, using a custom once implementation.

@js-choi
Copy link
Collaborator Author

js-choi commented Apr 4, 2022

@ckknight: Indeed, there is an issue devoted to this question, #5.

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. But let’s talk about that in #5.

@littledan
Copy link
Member

I was expecting "always-undefined", but if the ecosystem has found that caching the first answer is more convenient, then that's fine with me too (it does seem like a 0-ary memo in that case, though the caching policy is more straightforward to decide on).

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