medikoo / memoizee

Complete memoize/cache solution for JavaScript
ISC License
1.73k stars 61 forks source link

What about using dynamic arguments length by default #69

Closed epayet closed 7 years ago

epayet commented 7 years ago

Hi @medikoo

I know that By default, fixed number of arguments that function take is assumed (it's read from function's length property). I had an interesting case where that did not make sense for me, let me explain.

I'm using ES6, and transpile to ES5 using babel as many people do. I have a function which takes an optional parameter called options. It is an object and might contain a parser function:

function parseStr(str, options={}) {
    if(options.parser) {
        return options.parser(str);
    } else {
        return str;
    }
}

I naively thought that when I memoize this, memoizee will figure out that my function takes 2 parameters. As I was using it, memoizee did not care about my second parameter at all.

It took me a while to find out that babel is transpiling my original code to this:

function parseStr(str) {
    var options = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : {};

    if (options.parser) {
        return options.parser(str);
    } else {
        return str;
    }
}

So what happens is that my second argument gets stripped out by babel, and memoizee, using by default function.length to figure out the number of parameters, completely ignores the second one (not declared in that case but retrieved dynamically in arguments).

After understanding what's happening under the hood, I got it working by using length: false to say to memoizee that I might not know the number of arguments after all (length: 2 would also work in my case).

My thoughts are that to me, it makes much more sense that the library should figure out dynamically by default the number of args (as the ./normalizers/get.js is doing), instead of using function.length which is very tricky and can be wrong. Unless there is another reason to do that separation, like performance?

What do you think? :)

epayet commented 7 years ago

Again, I find it great to have the library configurable, I just wonder what default option makes more sense. Hope it can help 👍

medikoo commented 7 years ago

Babel does the right thing, as optional arguments are not counted in function's length by spec, and it's also the case in ES5, where optional arguments are not listed in function signature, but usually are defined as e.g.:

function parseStr(str/*, options={}*/) {
    var options = Object(arguments[1]);
    ...
}

Again, I find it great to have the library configurable

Yes, length option should be used if default behavior is not intended.

And answering the question, I don't think that changing default is a good idea. There are not many good use cases for length: false, also I'm not convinced yours is the right one. Mind that with length: false all following cases will produce different cache hits:

Generally I think your case will work best with custom normalizer, written as something as e.g.:

memoize(parseStr, { normalizer: function (args) {
    return JSON.stringify([args[0], args[1] || {}]);
} });
epayet commented 7 years ago

Thanks for your answer, it makes more sense now. You want to keep length: false as a specific case. I also get that it might have some side effects on changing the default behaviour.

Playing with default arguments is quite tricky! To work with that, one needs to understand what it becomes with ES5, and decide what to do with it (either length: false or ever better, use a custom normalizer as you mentioned to avoid the undefined, empty objects, etc. duplication).

Thanks for your time.