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

Exposing more isXyz helpers through node_util.cc. #7196

Closed
jdalton opened this issue Jun 7, 2016 · 14 comments
Closed

Exposing more isXyz helpers through node_util.cc. #7196

jdalton opened this issue Jun 7, 2016 · 14 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module.

Comments

@jdalton
Copy link
Member

jdalton commented Jun 7, 2016

I noticed that node/src/node_util.cc has

#define VALUE_METHOD_MAP(V)                                                   \
  V(isArrayBuffer, IsArrayBuffer)                                             \
  V(isDataView, IsDataView)                                                   \
  V(isDate, IsDate)                                                           \
  V(isMap, IsMap)                                                             \
  V(isMapIterator, IsMapIterator)                                             \
  V(isPromise, IsPromise)                                                     \
  V(isRegExp, IsRegExp)                                                       \
  V(isSet, IsSet)                                                             \
  V(isSetIterator, IsSetIterator)                                             \
  V(isTypedArray, IsTypedArray)

Which is great!

Is there interest in filling it out with IsWeakMap, IsWeakSet, IsProxy, IsSharedArrayBuffer,
or others?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 7, 2016

I'm in favor of this, but I know others are not. If you don't mind using a native addon, there is https://github.com/evanlucas/v8is.

@jasnell
Copy link
Member

jasnell commented Jun 7, 2016

I had attempted to add IsProxy in a recent PR and there was pushback. Like @cjihrig I'd be in favor of this but I'm not sure there is enough consensus.

@jdalton
Copy link
Member Author

jdalton commented Jun 7, 2016

Cool. I recently adopted these in Lodash too.

@jasnell
Copy link
Member

jasnell commented Jun 7, 2016

Ok. Just keep in mind that we tend to treat things on process.binding() as internal and subject to change.

@jdalton
Copy link
Member Author

jdalton commented Jun 7, 2016

Yep, yep. Usage is all guarded, checked, & yada yada.

Is there a way to iterate over the V8 Value methods and export the IsXyz ones? That way there's less to manually maintain and as new Value methods are added they're automagically exposed.

@jasnell
Copy link
Member

jasnell commented Jun 7, 2016

that would be a bit of a hard sell in light of this. There's an ongoing discussion about whether any of the is* methods should continue to be part of the public API

@jdalton
Copy link
Member Author

jdalton commented Jun 7, 2016

that would be a bit of a hard sell in light of this. There's an ongoing discussion about whether any of the is* methods should continue to be part of the public API

I think that fits fine with this. I'm asking to expand the node_util bindings not the public util module.

@mscdex mscdex added util Issues and PRs related to the built-in util module. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 7, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jul 5, 2016

I think expanding the bindings layer for things that aren't used in core will be an even harder sell. That said, I still think we should expose the type checking functions that would otherwise require a native addon.

@Trott
Copy link
Member

Trott commented Jul 8, 2017

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

@Trott Trott closed this as completed Jul 8, 2017
@jdalton
Copy link
Member Author

jdalton commented Jul 8, 2017

@jasnell Is there anything moving on utils/built-ins on the TC39 front?

@cjihrig
Copy link
Contributor

cjihrig commented Jul 22, 2017

It looks like https://github.com/jasnell/proposal-istypes is going to support type masquerading (see jasnell/proposal-istypes#30). For that reason, I'd like to see more of these bindings made available.

@jasnell
Copy link
Member

jasnell commented Jul 22, 2017

Yes, we're making progress on the proposal... keep in mind that it's still Stage-0 but the goal is get it to Stage-1 or even Stage-2 by around September. Won't be too difficult for us to polyfill either.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 22, 2017

For me personally, the whole point is to get a data type that cannot be faked anywhere without installing a compiled module.

@jdalton
Copy link
Member Author

jdalton commented Jul 22, 2017

I dig these utils for their perf wins. I'd be OK if values with @@builtin, or whatever symbol is settled on, hit a slow path though. I figure if folks want to say a value is duck, then it's their responsibility to ensure it quacks like one or things might get funky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

5 participants