prevent return "undefined" immediately when key is "constructor" for memoize method

[dacoozheng]

when key is "constructor", method after memoized will return "undefined" immediately.

var async = require('async');
var hardAsyncMethod = function(name, callback){
    setTimeout(function(){
        callback(null, 'hello, ' + name);
    }, 5000);
};
var memoizedHardAsyncMethod = async.memoize(hardAsyncMethod);
memoizedHardAsyncMethod('normalString', function(error, results){
    // normal process
    console.log(error);
    console.log(results);
});
memoizedHardAsyncMethod('constructor', function(error, results){
    // will return undefine immediately
    console.log(error);
    console.log(results);
});

[aearly]

😮 Can you add a test case for this?

[aearly]

Another way to fix this would be to do:

var memo = Object.create(null)

It'll still fail with memoizedHardAsyncMethod("__proto__") otherwise (but these are extreme edge cases).

[dacoozheng]

aearly, it is my first "pull request", and sorry for that I am not very clear with the process, and I have added two test cases for it, and these two test cases can pass but it turn out another CI issue cannot pass "Node.js 5" of "retry as an embedded task with interval", saying "Assertion Message: The duration should have been greater than 400, but was 399". Actually I didn't change that test case, would you kindly help to give some hint for me to fix or any other advice? thanks a lot!

[megawac]

The one possible issue is when key is another annoyign case "hasOwnProperty". Anyway, I'm +1 for this under the impression the hasOwnProperty is changed to _.has with #996.

@dacoozheng looks good for the most part, just some indentation issues.

[dacoozheng]

@megawac I refine it to allow "hasOwnProperty" as key.

[megawac]

just fix this indentation and it looks fine

[qsona]

I prefer var memo = Object.create(null) as @aearly pointed out. Specifying __proto__ is certainly an edge case, but I'm afraid it might be specified by end-user and the application might be vulnerable. Object.create(null) is a best way to get a plain hash in ~ES5.

[megawac]

Problem is that async ~1.0 is es3 compatible. We could do create in v2
On Jan 4, 2016 2:30 AM, "qsona" [email protected] wrote:

I prefer var memo = Object.create(null) as @aearly
https://github.com/aearly pointed out. Specifying proto is
certainly an edge case, but I'm afraid it might be specified by end-user
and the application might be vulnerable. Object.create(null) is a best
way to get a plain hash in ~ES5.

—
Reply to this email directly or view it on GitHub
#998 (comment).

[dacoozheng]

@qsona yes, if we just need a plain hash (Say, like a map), Object.create(null) should be a better way in ~ES5.

var memo = Object.create(null) means create a plain object and not inherit from Object.prototype, so there are no attributes and methods from Object.prototype, Say, "toString/toLocalString", "hasOwnProperty", "constructor", "isPrototypeOf", "valueOf", ...etc.

And solution is always responding to the requirement and situation, imagine below case:
var aFunc = function(name, callback){// an asynchronours method...};
var aMemoizedFunc = async.memoize(aFunc);
var aMemo = aMemoizedFunc.memo;
From the API, the async consumer can use or modify "aMemo" (e.g. aMemo.key = value or something others), and what's the expectation from the consumer think "aMemo" as what (a javascript object)? and some third-party lib or consumer used async may use below javascript idiom:
aMemo.hasOwnProperty("...") or console.log(aMemo + "") or var someOthers = Object.create(aMemo), but actually "aMemo" cannot support it if use Object.create(null).

I am just thinking, return a normal javascript object maybe more better except document it explicitly in API document (Saying, the "memo" attribute is an Object.create(null)).

[qsona]

@megawac I completely forgot that Object.create is a feature of ES5...
@dacoozheng I didn't regard aMemoizedFunc.memo as a public API, but I should rethink because it is exposed and may be used by someone.

Thanks for pointing them out. I lacked the consideration. I also checked the memoize implementation of lodash but it may be too complicated for carrying to async.
+1 for this change for now.

[aearly]

I think, hasOwnProperty is good enough for now. We can use Object.create(null) in v2.