jasnell / proposal-istypes

TC-39 Proposal for additional is{Type} APIs
201 stars 7 forks source link

Why not Reflect.is{Type}? #14

Closed mgtitimoli closed 7 years ago

mgtitimoli commented 7 years ago

Hi James,

Before going further, I wanted to thank you for bringing these additions to discussion :slightly_smiling_face:

Since the addition of Reflect, I was wondering if all the stuff that does some kind of reflection should be added under this one.

I know we already have Array.isArray, but should we continue moving forward on this direction, or with this that you are proposing is a good time to branch to this other alternative?

ljharb commented 7 years ago

What would "Type" be though? If the literal value of, say, String, then what about cross-realm values? What about when the global has been polyfilled/replaced, as es6-shim does with RegExp and Number?

An API that accepts an enum of string values (like "String") would not be a good idea, because stringly typed APIs have a host of problems.

ljharb commented 7 years ago

Separately, Reflect is for, and only for, a 1:1 correspondence to Proxy traps - which is why when the [[Enumerate]] trap was removed, Reflect.enumerate was removed also. Thus, even if this API was a single function, it couldn't ever live on Reflect.

mgtitimoli commented 7 years ago

Hi Jordan,

What I'm proposing is instead of attaching functions to all the constructors, why don't we just add all of them under Reflect... I know that its intention until now is (quoting from MDN)

Reflect is a built-in object that provides methods for interceptable JavaScript operations. The methods are the same as those of proxy handlers. Reflect is not a function object, so it's not constructible.

But, its addition opened a new alternative to place all the Reflection API under this single object, this is what I'm saying, Reflect.isArray, Reflect.isObject, Reflect.isString, and so on...

ljharb commented 7 years ago

The committee has flatly refused many times to add anything to Reflect that isn't a Proxy trap, so that's a nonstarter. See "The methods are the same as those of proxy handlers".

mgtitimoli commented 7 years ago

I understand your point, but going through this direction will allow to add checkers of stuff that has no associated constructors like generators for example, that correct me if I'm wrong, you were discussing to add them under Object, which has no sense.

mgtitimoli commented 7 years ago

Also, IMHO people that land to JavaScript would expect, due the fact that there is an object called Reflect, to find all the Reflection API under this one, and not spread all over the entire set of available constructors/objects

ljharb commented 7 years ago

I don't know how to state any clearer that these will never be added to Reflect, since they are not Proxy traps.

The two that are in the proposal to be added to Object are for isGeneratorObject, and isObject. Personally I don't feel that isObject is useful unless it returned true for any non-primitive (which would not be the same as "instanceof Object"), and isGeneratorObject could only be typeof obj.next === 'function' which would give many false positives. The reasons these don't make sense are related to what they are checking for, and not to which constructor they live on.

mgtitimoli commented 7 years ago

I don't know how to state any clearer that these will never be added to Reflect, since they are not Proxy traps.

It's OK, there is no need to reply this way, I was trying to express my point of view.

And answering your observation of Object.isObject, lots of people since the appearance of the language, myself included, are constantly doing:

thing !== null && typeof thing === "object"

This is an excellent opportunity to fix this.

jasnell commented 7 years ago

@ljharb brings up several interesting points that simply underscore how difficult reliably addressing this problem is and I definitely agree that Reflect is not the right home for these kinds of functions. In many cases, the checks are a best guess at best.

jasnell commented 7 years ago

That said, the utility of such checks has been demonstrated repeatedly in various use cases and Node.js core itself depends on stronger type checking as part of it's type inspection mechanism.

mgtitimoli commented 7 years ago

Just to mention some of the reasons of why I have created this issue:

ljharb commented 7 years ago

@mgtitimoli the other proposal has not yet been proposed to the committee, but it would not be able to add those methods to Reflect for the same reason.

jasnell commented 7 years ago

+1 to what @ljharb is saying here. Using Reflect would not make sense given the intended use of that API.