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

“Check for plain object” may be have a problem #114

Closed
ustccjw opened this issue Apr 26, 2014 · 8 comments
Closed

“Check for plain object” may be have a problem #114

ustccjw opened this issue Apr 26, 2014 · 8 comments

Comments

@ustccjw
Copy link

ustccjw commented Apr 26, 2014

function isObject(val) {
  return val && Object == val.constructor;
}

how about:

function isObject(val) {
  return val && 'Object' == val.constructor.name;
}

or like in browser:

function isObject(val) {
  return val && Object.prototype.toString.call(val) === '[object Object]';
}
@juliangruber
Copy link
Contributor

is that really a problem?

@yanickrochon
Copy link

The second "like in browser" will slow down the code as call is slower than a direct function call, plus, the current implementation does not require a function call at all (meaning, it's faster).

You should not yield anything else than a Promise, a GeneratorFunction, a Function or a plain Object anyhow. So, when you say "may have a problem", what kind of problem do you have in mind? Can you give any example of a potential issue with the current implementation? I am all in favor of preventive maintenance, but if it creates bottle necks and does not improve the code in any way...

@wilmoore
Copy link

return val && 'Object' == val.constructor.name;

This one should fail if someone creates a constructor like this:

var Ctor = function () {}

Because .name will not exist. This can be fixed by changing the expression to var Ctor = function Ctor() {}; however, it is likely that it you'll see the first version instead.

@yanickrochon
Copy link

@wilmoore , no, it will not fail, as co is looking if the yielded object context is a function before it checks if it's an Object, So, no problem here.

@wilmoore
Copy link

as co is looking if the yielded object context is a function before

OH, cool. Well, that's fine then.

@ustccjw
Copy link
Author

ustccjw commented Apr 27, 2014

@juliangruber @yanickrochon if the object is just a plain object, that's ok.

If i want to yield a object through a constructor(eg. function A(a) {this.a=a}, yield new A('aaa')),
i think Object.prototype.toString.call(val) === '[object Object]' is always okey.

ps. Actually, constructor can be modified by you, so i think it is not safe. That's the reason that you use Array.isArray(val) instead of Array == val.constructor.

@ustccjw ustccjw closed this as completed Apr 27, 2014
@ustccjw ustccjw reopened this Apr 27, 2014
@navaru
Copy link
Contributor

navaru commented Apr 27, 2014

"If it ain't broke don't fix it"

Is there a use case where this fails?

@ustccjw ustccjw closed this as completed Apr 27, 2014
@tj
Copy link
Owner

tj commented May 3, 2014

yielding non-plain objects would be pretty weird and likely break in most cases, it's a pretty questionable feature in the first place haha, handy, but questionable

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

No branches or pull requests

6 participants