feat(util.is()): introduce
#119
Conversation
| if (someValue instanceof Date) { | ||
| console.log('someValue is a date'); | ||
| } | ||
| if (someValue instanceof Error) { |
There was a problem hiding this comment.
| if (someValue instanceof Error) { | |
| if (Error.isError(someValue)) { |
There was a problem hiding this comment.
Let's check the behavior of the original function to be sure.
There was a problem hiding this comment.
Good to check, but this is still the more correct future-facing alternative.
| if (typeof someValue === 'number') { | ||
| console.log('someValue is a number'); | ||
| } | ||
| if (typeof someValue === 'object' && someValue !== null) { |
There was a problem hiding this comment.
what about functions, which are also objects?
| if (Buffer.isBuffer(someValue)) { | ||
| console.log('someValue is a buffer'); | ||
| } | ||
| if (someValue instanceof Date) { |
There was a problem hiding this comment.
instanceof is not reliable and can be forged; i'm not sure this is a good replacement to be recommending
| if (someValue !== Object(someValue)) { | ||
| console.log('someValue is a primitive'); | ||
| } | ||
| if (someValue instanceof RegExp) { |
There was a problem hiding this comment.
instanceof is not reliable and can be forged; i'm not sure this is a good replacement to be recommending
|
@AugustinMauroy Can you check the original implementations and make sure these match them? If they do, can you make that clear? |
|
@avivkeller & @ljharb I just applied what the docs said. So I will not change it for now. Here docs ref
|
| @@ -34,7 +34,7 @@ export default function transform(root: SgRoot): string | null { | |||
| 'isBoolean': (arg: string) => `typeof ${arg} === 'boolean'`, | |||
| 'isBuffer': (arg: string) => `Buffer.isBuffer(${arg})`, | |||
| 'isDate': (arg: string) => `${arg} instanceof Date`, | |||
| 'isError': (arg: string) => `${arg} instanceof Error`, | |||
| 'isError': (arg: string) => `Object.prototype.toString(${arg}) === '[object Error]' || ${arg} instanceof Error `, | |||
There was a problem hiding this comment.
| 'isError': (arg: string) => `Object.prototype.toString(${arg}) === '[object Error]' || ${arg} instanceof Error `, | |
| 'isError': (arg: string) => `Object.prototype.toString.call(${arg}) === '[object Error]' || ${arg} instanceof Error `, |
There was a problem hiding this comment.
https://nodejs.org/api/deprecations.html#dep0048-utiliserror
The
util.isError()API has been removed. Please useObject.prototype.toString(arg) === '[object Error]' || arg instanceof Error instead.
There was a problem hiding this comment.
That's just wrong though, it won't work. It shouldn't matter what's in the docs - putting this in the codemod will break projects.
There was a problem hiding this comment.
Humm idk what should we do I always follow doc. @JakobJingleheimer (the formal maintainer) what is your opinon
| 'isError': (arg: string) => `Object.prototype.toString(${arg}) === '[object Error]' || ${arg} instanceof Error `, | ||
| 'isFunction': (arg: string) => `typeof ${arg} === 'function'`, | ||
| 'isNull': (arg: string) => `${arg} === null`, | ||
| 'isNullOrUndefined': (arg: string) => `${arg} === null || ${arg} === undefined`, |
There was a problem hiding this comment.
this is the same as ${arg} == null, and it's simpler, so maybe we should stick with that?
There was a problem hiding this comment.
https://nodejs.org/api/deprecations.html#DEP0051
The
util.isNullOrUndefined()API has been removed. Please usearg === null || arg === undefinedinstead.
| 'isNullOrUndefined': (arg: string) => `${arg} === null || ${arg} === undefined`, | ||
| 'isNumber': (arg: string) => `typeof ${arg} === 'number'`, | ||
| 'isObject': (arg: string) => `${arg} && typeof ${arg} === 'object'`, | ||
| 'isPrimitive': (arg: string) => `${arg} === null || (typeof ${arg} !=='object' && typeof ${arg} !== 'function')`, |
There was a problem hiding this comment.
also, Object(${arg}) !== ${arg} is terser
There was a problem hiding this comment.
https://nodejs.org/api/deprecations.html#dep0054-utilisprimitive
the
util.isPrimitive()API has been removed. Please usearg === null || (typeof arg !=='object' && typeof arg !== 'function')instead.
|
I'm sorry @ljharb but If you think there are better way to handle that please first update node api doc and then we will update this codemod |
|
For the opinion ones, fine, but one of them is a bug and is just broken. It shouldn't matter what's in the docs, but it matters what code does. |
I'm annoyed because I'm caught between two stools. On the one hand, I agree with you. On the other, I want to follow the documentation strictly so that the org node is consistent. |
|
That seems like an arbitrary and counterproductive constraint to me. The docs aren’t the source of truth, they need to reflect the actual source of truth, the code. |
Description
Close #116