Aimed to fix issues #1, #2 and #3. The argument Array conversion is neede

[JamieMason]

Aimed to fix issues #1, #2 and #3. The argument Array conversion is needed now though for the JSON.stringify.

[addyosmani]

Thanks for the pull request, Jamie!

Just to confirm: have the issues tackled been tested? If so, could you share the tests you used (even if its simple a usage test sans unit testing) just so we can evaluate the changes further?

If this ends up getting merged, I'll update the original post on memoize.js to reflect the improvements made through this patch (and of course credit your contributions).

[JamieMason]

Of course, thanks Addy. As it was a quick help-out, I just tested by running memoize against @cowboy's example - no unit tests I'm afraid sorry.

/*
* memoize.js
* by @philogb and @addyosmani
* with further optimizations by @mathias
* @DmitryBaranovsk and @GotNoSugarBaby
* perf tests: http://bit.ly/q3zpG3
* Released under an MIT license.
*/
function memoize(fn) {
   "use strict";
   var cache = (fn.memoize = fn.memoize || {}),
       stringifyJson = JSON.stringify,
       sliceArray = Array.prototype.slice;

    return function () {
        var hash = stringifyJson(sliceArray.call(arguments));
        return (hash in cache) ? cache[hash] : cache[hash] = fn.apply(this, arguments);
    };
}

// @cowboy's test function
function testMemoize(a, b) {
  return a * 10 + b;
}

console.log(testMemoize(11, 1)); // 111
console.log(testMemoize(1, 11)); // 21

var memo = memoize(testMemoize);
console.log(memo(11, 1)); // 111
console.log(memo(1, 11)); // 21

[addyosmani]

No worries! We'll put something together. As long as it works against the examples he was trying before, we've got a good starting point to work from. Thanks for your contributions once again!

[JamieMason]

Also, this should explain keeping the Array.prototype.slice;

function noArgClone ()
{
    console.log(JSON.stringify(arguments));
}

function argClone ()
{
    console.log(JSON.stringify(Array.prototype.slice.call(arguments)));
}

argClone(1,2,3); // [1,2,3]
noArgClone(1,2,3); //{"0":1,"1":2,"2":3}

[JamieMason]

No problem, I enjoyed it

[DmitryBaranovskiy]

Should we add try/catch around JSON.stringify? There is a case when somebody passes cyclic structure to the function. It could not be able to cache it, but at least it should return the value.

[DmitryBaranovskiy]

Also would be nice to add optional scope argument, so it will fn.apply(scope || this, arguments);

[addyosmani]

@DmitryBaranovskiy I agree it might be useful to add the optional scope argument. With respect to the try/catch, I imagine cyclic structures being passed as being an edge case - do you think there would be enough people accidentally passing that through to warrant the change?

[addyosmani]

As both of you have suggested optimizations for the memoizer that have ended up being merged (and appear quite interested in taking this work even further with me), I've added you as collaborators to the repo.

I think for now the focus might be incremental changes that improve performance/needs to take it up to production level. I personally would like to keep benchmarks up to date and begin better testing with older browsers as a few people have mentioned issues with IE8/9, but the unit tests will certainly help with that.

[JamieMason]

Closed pull request, as the change made it into the repo via commit a159955fef736fe6bb2ef7ca70b09b5f304edbf9

[addyosmani]

Just to confirm: are you happy enough with the patches made to address issues 1, 2 and 3 for us to close those issues too?. Cheers!

[JamieMason]

I did think about closing them, but thought I'd better leave them for @cowboy as he'd raised them.

[addyosmani]

He's usually quite busy, but I'll ping him on IRC and see if he's satisfied those points have been addressed :)

[JamieMason]

Sure

[AutoSponge]

"Also, this should explain keeping the Array.prototype.slice;"

Well, how the cache keys "look" doesn't deter me from removing the function call. The performance of the cache is more important than something I don't see. I still prefer the version without slice.

[JamieMason]

@AutoSponge - Crikey yeah of course - you're right! It doesn't matter at all in this case that it's not actually an Array, we just want a unique, reproducible key. I just went "oh, not an Array, better fix it" without really thinking. Thanks again Paul.