Skip to content
This repository has been archived by the owner on Jul 31, 2018. It is now read-only.

Add 'types' module to core #44

Closed
wants to merge 1 commit into from
Closed

Conversation

evanlucas
Copy link

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
Copy link
Member

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

  • isSymbolObject
  • isFloat32x4 (no idea why this is only present for one of the SIMD types)
  • isSharedArrayBuffer

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

@evanlucas
Copy link
Author

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

@jasnell
Copy link
Member

jasnell commented Sep 22, 2016

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

  • On isProxy, for instance, I had to introduce a mechanism for getting the proxy internals as opposed to simply determining if the object is a proxy (see the code added to util.inspect() with regards to Proxy handling). For some of these, simply getting a boolean back isn't enough and making two calls to the V8 layer for the details isn't performant enough.
  • I think I'd much rather these remain on the existing util module. I know those have been deprecated for a while but... I don't think they should be (I may have agreed with it at some point, I don't know, but now, not so much).

@cjihrig
Copy link

cjihrig commented Sep 22, 2016

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

@Fishrock123
Copy link
Member

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

@evanlucas
Copy link
Author

@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
Copy link
Member

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
Copy link
Member

and Chakra might appreciate not needing MORE shims?

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

@jasnell
Copy link
Member

jasnell commented Sep 26, 2016

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
#44 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2ebEBgpFNXEMqpHjNtq08L4ggMEOQks5quENlgaJpZM4KD4bw
.

@kunalspathak
Copy link
Member

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
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Sep 28, 2016

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
Copy link
Member

jasnell commented Sep 28, 2016

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

@Fishrock123
Copy link
Member

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
Copy link
Member

jasnell commented Sep 28, 2016

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
Copy link

cjihrig commented Sep 28, 2016

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
Copy link
Member

jasnell commented Sep 28, 2016

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
Copy link
Member

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
Copy link

cjihrig commented Sep 28, 2016

+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
Copy link

mikeal commented Sep 28, 2016

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
Copy link
Member

jdalton commented Sep 28, 2016

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
Copy link
Member

jasnell commented Sep 28, 2016

@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
Copy link
Member

jasnell commented Sep 28, 2016

@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
Copy link

disnet commented Sep 28, 2016

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
Copy link
Member

jasnell commented Sep 28, 2016

@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
Copy link
Member

Fishrock123 commented Sep 28, 2016

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
Copy link
Author

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

@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

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
Copy link
Member

jasnell commented Oct 10, 2016

See: tc39/proposals#29

done via a native addon [(v8is)](https://github.com/evanlucas/v8is), it
requires that a compiler be installed on the consumer's machine.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@styfle
Copy link
Member

styfle commented Mar 19, 2018

I haven't seen any movement on this.

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

@evanlucas
Copy link
Author

Closing as this has been superseded by nodejs/node#18415

@evanlucas evanlucas closed this Mar 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet