util: fix constructor/instanceof checks

[mscdex]

util: fix constructor/instanceof checks

These new checks are similar to the one introduced in 089d688, but for other types of objects. Specifically, if an object was created in a different context, the constructor object will not be the same as the constructor object in the current context, so we have to compare constructor names instead.

[evanlucas]

LGTM. is there anywhere else we are relying on similar functionality?

[mscdex]

Yeah, I guess I'll fix those up too while I'm in there.

[lucthev]

Should this be typeof obj !== 'undefined' or obj !== undefined, like above?

[mscdex]

Updated to fix the other constructor/instanceof checks.

CI: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/515

[evanlucas]

Just curious, but I wonder if the isPromise, etc are faster than v8::Value::IsPromise. I would assume so, since they aren't doing the call into c++, but not sure

[mscdex]

Updated again to fix typo. CI: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/516

[vkurchatkin]

This doesn't seem better than Object.prototype.toString

[mscdex]

@vkurchatkin That won't work for Promises though. I've now replaced isMap() and isSet().

[targos]

@mscdex I don't know if Map and Set can be subclassed, but I think your first version had the advantage to catch this:

class MyMap extends Map {}
util.inspect(new MyMap)

[mscdex]

@targos They can be and apparently Object.prototype.toString() works for subclasses too. There were already separate tests for those cases.

CI after replacing Map/Set check implementations: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/517/

[evanlucas]

I'm still a little concerned about using objectToString. I thought that wasn't as reliable?

[mscdex]

@evanlucas Unreliable in what way? We're already using the same method in other util methods for checking for other types such as Date, RegExp, and Error.

[evanlucas]

maybe I'm thinking about once @@toStringTag lands in v8...I could have sworn I remember seeing that it was not reliable...nevermind though

[evanlucas]

LGTM if CI is happy

[jasnell]

As much as depending on objectToString bothers me, I'm not sure there's a better way. LGTM so long as CI is happy

[mscdex]

@jasnell Well, previously I was walking the prototype chain (similar to what I'm currently doing for checking for promises), but I'm not sure what the performance differences are.

[jasnell]

Yeah, like I said, I'm not sure there's a better way. It just makes me sad to be forced to rely on toString checks

[jasnell]

Kicked off another CI run, just to be sure since the last one came up red: https://ci.nodejs.org/job/node-test-pull-request/567/

[jasnell]

@mscdex .. hmm... thinking... would this qualify as a semver-minor or major? it likely shouldn't but want to be sure

[mscdex]

Wow, looks like there's something missing from a previous commit... an addon test is looking for a debug module...

[mscdex]

Personally I would see this as a bug fix, so I wouldn't see it as a semver-major. Not sure whether it'd be a semver-minor or not though.

[jasnell]

Very odd... testing a clean build locally on osx...

[jasnell]

@mscdex ... not seeing the same failure locally on osx ... can you test on your end?

[trevnorris]

FWIW calling into native is cheap, the call itself is simple enough:

void CheckIsPromise(const FunctionCallbackInfo<Value>& args) {
  args.GetReturnValue().Set(args[0]->IsPromise());
}

And this method is more definitive.

[jasnell]

New CI: https://ci.nodejs.org/job/node-test-pull-request/676/

@mscdex ... did you see the last comment from @trevnorris ?

[mscdex]

@jasnell I've removed the js isPromise() and am just calling inspectPromise() every time which already does a C++-based promise check like @trevnorris suggested.

[targos]

this comment isn't relevant anymore

[mscdex]

What do you mean? A mirror is still only created if it's been determined the object is a promise. Do you mean the "superficially" part?

[targos]

Yes, now the check isn't superficial. Also the comment isn't in the right context. It is not obvious here that we create an object mirror.
I know the change was made earlier but do you mind moving it to the inspectPromise declaration ?

[jasnell]

I personally think it's fine the way it is.

[jasnell]

LGTM. CI is red but the failures do not appear to be related.

[mscdex]

@targos I've updated the comment about Promises. LGTY?

[mscdex]

I ran through CI one more time and only saw a few unrelated errors on Windows.

[targos]

LGTM

[mscdex]

Landed in 82b8355.