nodejs / node-eps

Node.js Enhancement Proposals for discussion on future API additions/changes to Node core
443 stars 66 forks source link

Add 'types' module to core #44

Closed evanlucas closed 6 years ago

evanlucas commented 7 years ago

Description

This proposal is to add a builtin module that provides a way to determine whether the passed argument is of a particular type. Although this can be done via a native addon (v8is), it requires that a compiler be installed on the consumer's machine.

We can expose these methods using currently by using v8::Value.

The util.is* functions are currently deprecated in the documentation. This proposal will add an upgrade path to those currently using those functions.

Proof-of-concept

A proof-of-concept (does not include docs or a full test suite yet) can be found at:

https://github.com/evanlucas/node/tree/types


I know some have expressed interest in this and I wanted to extend a formal proposal for implementing.

/cc @cjihrig @jasnell @jdalton

addaleax commented 7 years ago

One may want to add, according to the most recent v8.h in node:

(I assume the plan is keeping the list more or less up to date with what V8 offers?)

evanlucas commented 7 years ago

@addaleax thanks! Yes, the plan was to keep the list updated with what v8 offers.

jasnell commented 7 years ago

While I'm fine with this in general, I do have some concerns:

cjihrig commented 7 years ago

I'm in favor of exposing util.is* in a way that is in sync with what V8 offers.

Fishrock123 commented 7 years ago

I really think we should just talk to people from TC39 about putting this in the language itself...?

evanlucas commented 7 years ago

@Fishrock123 while I agree 100%, I would assume the timeline on that would be quite a bit longer than if we were to go this route for the time being.

williamkapke commented 7 years ago

This would be cool. Getting it in the language would certainly be preferable. ... BUT I just want to note that won't be "v8 offerings" ... Which is probably a better anyway since we are considering having Node be VM neutral. ... and Chakra might appreciate not needing MORE shims?

... but I'd really like having it.

addaleax commented 7 years ago

and Chakra might appreciate not needing MORE shims?

Maybe it’s worth to ping @nodejs/chakracore?

jasnell commented 7 years ago

Putting such a proposal forward is not difficult. Convincing the commitee that it's worthwhile is much more difficult. Is there a sufficient case that could be made for why these would be generally useful in the browser?

On Monday, September 26, 2016, Anna Henningsen notifications@github.com wrote:

and Chakra might appreciate not needing MORE shims?

Maybe it’s worth to ping @nodejs/chakracore https://github.com/orgs/nodejs/teams/chakracore?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nodejs/node-eps/pull/44#issuecomment-249713132, or mute the thread https://github.com/notifications/unsubscribe-auth/AAa2ebEBgpFNXEMqpHjNtq08L4ggMEOQks5quENlgaJpZM4KD4bw .

kunalspathak commented 7 years ago

and Chakra might appreciate not needing MORE shims?

Chakra would still need to shim out calls to Is*() methods, the way it does today. There won't be any change in shim layer by adding this in core.

Fishrock123 commented 7 years ago

@jasnell same reasons as we use them? I'm not seeing why there's much difference.

jasnell commented 7 years ago

That doesn't always fly. For instance, there's extremely little reason why a browser developer would need isProxy(). The only reason we need to be able to determine if an object is a Proxy is in a very specific edge case when using util.inspect() to generate some debug output. We have to be able to articulate a clear benefit to adding these checks to the language vs. simply implementing them as part of the host environment and for most of these I'm not convinced the use case is quite compelling enough.

jasnell commented 7 years ago

That said, since I'm here this week anyway I'll ask :-)

Fishrock123 commented 7 years ago

The only reason we need to be able to determine if an object is a Proxy is in a very specific edge case when using util.inspect() to generate some debug output.

Then why would we want to publicly expose it ourselves? There is a reason there are deprecated in the docs.

jasnell commented 7 years ago

IsProxy is not exposed ;-) ... it's only used internally. Well, actually, not even that, there's an internal process.binding for getting the internals of a Proxy object because it's too slow to do an IsProxy first. Looking over the list, I'm certainly not convinced that there would be a definitive need to expose all of these Is* statements, although I'm ok with the basic idea in general.

cjihrig commented 7 years ago

There is a reason there are deprecated in the docs.

Yes, supporting a function like isNull() that literally only checks if a value === null is silly. That's not the case for checks that can't be reliably done in JS and require a C++ compiler though.

jasnell commented 7 years ago

Ok, here's an idea for a proposal to TC-39. There are already examples of similar types of methods (e.g. Array.isArray()). We can take these and do similar things with the other intrinsic types... e.g. Proxy.isProxy(), Symbol.isSymbol(), etc. I'll put it together and float it as a proposal to TC-39 in the near future.

kunalspathak commented 7 years ago

I agree with @jasnell . Proxy.isProxy() should come through TC-39 spec. When this API was introduced in util.inspect(), I tried various javascript ways to shim it but it was not possible to do it without native support. Currently it is still a TODO work item. in chakrashim.

cjihrig commented 7 years ago

+1 to a proposal to TC-39. In the event that it is rejected, or takes a long time to play out, it is still useful for Node to provide this though.

mikeal commented 7 years ago

It's worth noting that TC39 isn't done adding new types so part of this proposal would also be to include similar "is" methods on all new types so we don't continue to see this going forward.

jdalton commented 7 years ago

Probably worth noting the TC39 hasn't yet outlined the bar to meet for new built-ins (it's on a todo), so it's a bit hit-n-miss for what sticks and what doesn't.

jasnell commented 7 years ago

@jdalton ... it definitely would be good to have those constraints but we can make do for the time being by just bringing the proposal in and seeing how it goes.

jasnell commented 7 years ago

@cjihrig ... for a TC-39 proposal to make progress we'd have to provide an implementation so doing something in parallel makes sense. We'd just need to be deliberate about how we introduce the APIs to make sure they match up with what is being proposed.

disnet commented 7 years ago

Note that TC39 considered isProxy during the design of Proxies but intentionally chose not to include it to allow proxies “to hide their Proxy-ness” (see the notes for some small context).

jasnell commented 7 years ago

@disnet ... yes, I'm not convinced that Proxy.isProxy() is one that we'd really want to pursue in the proposal. I need to spend some time with this to pair the list down to ones that are particularly useful.

Fishrock123 commented 7 years ago

At the very least an official list of built-in language types would be useful so as to decide what we should or shouldn't do in this potential space if it comes down to it. (May already exist)

evanlucas commented 7 years ago

One use case I can think of for isProxy (and a lot of the others) is for formatting or logging

jasnell commented 7 years ago

Here is the start of a proposal for TC-39: https://github.com/jasnell/proposal-istypes I'll continue working on this a bit then get it submitted to the TC-39 process.

jasnell commented 7 years ago

See: https://github.com/tc39/proposals/pull/29

styfle commented 6 years ago

I haven't seen any movement on this.

@jasnell It looks like the TC39 proposal is still stage 0? 😕

evanlucas commented 6 years ago

Closing as this has been superseded by https://github.com/nodejs/node/pull/18415