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

util: deprecate most of util.is*() #6235

Closed
wants to merge 1 commit into from

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Apr 15, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

util

Description of change

Supersedes #1301

This deprecates most of util.is except some stuff for which v8 has internal functions for and primitives.

To be quite honest, I don't really know what to do here, if this is remotely right, etc.

cc @nodejs/ctc I guess

note: the tests currently log a ton of warnings but I can't be bothered to fix them until we decide on what is the best path forward

@Fishrock123 Fishrock123 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++. semver-major PRs that contain breaking changes and should be released in the next major version. discuss Issues opened for discussions and feedbacks. labels Apr 15, 2016
exports.isNullOrUndefined = isNullOrUndefined;
exports.isNullOrUndefined = internalUtil.deprecate(isNullOrUndefined,
'util.isNullOrUndefined() is deprecated. Use the following instead:\n' +
'arg === null || arg === undefined');
Copy link
Contributor

Choose a reason for hiding this comment

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

or arg == null

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on == null. Last I looked at the optimizing compiler nullOrUndefined is special cased.

@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

LGTM

@Fishrock123
Copy link
Contributor Author

Still not convinced this is actually 100% the right thing to do fwiw

@mscdex
Copy link
Contributor

mscdex commented Apr 19, 2016

@Fishrock123 You mean in general or specifically the c++ IsPrimitive() implementation?

@Fishrock123
Copy link
Contributor Author

@mscdex both, any of the above

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Apr 29, 2016
exports.isFunction = isFunction;
exports.isFunction = internalUtil.deprecate(isFunction,
'util.isFunction() is deprecated. ' +
'Use typeof arg === \'function\' instead.');

function isPrimitive(arg) {
Copy link
Member

Choose a reason for hiding this comment

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

I would separate out the change to isPrimitive to a separate commit or PR.

@jasnell
Copy link
Member

jasnell commented Apr 30, 2016

I still say +1 on this. With exception to isRegExp(), isDate() and isPrimitive(), these are of fairly little value. The new IsPrimitive check should likely be split out to a separate commit and reconciled with the new approach v8 is taking (#6235 (comment)) but I definitely think this is the right way to go.

@Fishrock123
Copy link
Contributor Author

@jasnell Problem is that presents no consistency at all.. it's hard to justify either way.. like what will we add for future things that v8 may have checks for?

I'm less and less convinced we should even expose the v8 checks. There must be reasonable ways to get this in JS otherwise JS would present checks for it for browsers...

cc @ljharb and @bmeck ... What is the recommended way of type checking these things in JS?

On our end these are most notably used for how we render in util.inspect().

@jasnell
Copy link
Member

jasnell commented May 2, 2016

the Proxy discussion is a good example of this. In the normal case, there's no reason for most users to need to check to see if an object is a Proxy but util.inspect() provides a valid use case for why we'd want to know if it is or not. However, I don't feel strongly about it. If you don't think there's value is having any of them, deprecate them all and don't add any new ones.

@cjihrig
Copy link
Contributor

cjihrig commented May 2, 2016

I'm still very much in favor of keeping the things that can't be done with 100% certainty in JavaScript without deferring to a native addon that does this same thing. Even if this is an inconsistent API, it will still be far from the worst thing in core.

@cjihrig
Copy link
Contributor

cjihrig commented May 2, 2016

Also, browsers don't have to account for things that cross the vm boundary.

@ljharb
Copy link
Member

ljharb commented May 2, 2016

@Fishrock123 https://npmjs.com/~ljharb - most everything starting with "is".

If utils.is remains, I'd prefer to see them do things correctly, cross-realm.

@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

Ping @Fishrock123 ... just checking in on this... what do you want to do with this? I'd recommend closing if it's not yet clear if this should happen.

@Fishrock123
Copy link
Contributor Author

no clue I guess the APIs will just sit in limbo forever
¯\_(ツ)_/¯

@Fishrock123 Fishrock123 closed this Mar 6, 2017
ryzokuken added a commit to ryzokuken/node that referenced this pull request Mar 13, 2018
Rename the tests appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the tests conform
to the standard test structure

- Rename test-regress-nodejsGH-9819 to test-crypto-tostring-segfault
- Rename test-regress-nodejsGH-5051 to test-http-addrequest-localaddress
- Rename test-regress-nodejsGH-5727 to test-net-listen-invalid-port
- Rename test-regress-nodejsGH-5927 to test-tty-stdin-pipe
- Rename test-regress-nodejsGH-6235 to test-v8-global-setter

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
lpinca pushed a commit that referenced this pull request Mar 13, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-9819 to test-crypto-tostring-segfault
- Rename test-regress-GH-5051 to test-http-addrequest-localaddress
- Rename test-regress-GH-5727 to test-net-listen-invalid-port
- Rename test-regress-GH-5927 to test-tty-stdin-pipe
- Rename test-regress-GH-6235 to test-v8-global-setter

PR-URL: #19275
Refs: #19105
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-9819 to test-crypto-tostring-segfault
- Rename test-regress-GH-5051 to test-http-addrequest-localaddress
- Rename test-regress-GH-5727 to test-net-listen-invalid-port
- Rename test-regress-GH-5927 to test-tty-stdin-pipe
- Rename test-regress-GH-6235 to test-v8-global-setter

PR-URL: #19275
Refs: #19105
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-9819 to test-crypto-tostring-segfault
- Rename test-regress-GH-5051 to test-http-addrequest-localaddress
- Rename test-regress-GH-5727 to test-net-listen-invalid-port
- Rename test-regress-GH-5927 to test-tty-stdin-pipe
- Rename test-regress-GH-6235 to test-v8-global-setter

PR-URL: #19275
Refs: #19105
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-nodejsGH-9819 to test-crypto-tostring-segfault
- Rename test-regress-nodejsGH-5051 to test-http-addrequest-localaddress
- Rename test-regress-nodejsGH-5727 to test-net-listen-invalid-port
- Rename test-regress-nodejsGH-5927 to test-tty-stdin-pipe
- Rename test-regress-nodejsGH-6235 to test-v8-global-setter

PR-URL: nodejs#19275
Refs: nodejs#19105
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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++. discuss Issues opened for discussions and feedbacks. semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled. util Issues and PRs related to the built-in util module. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants