-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
doc: deprecate utilsNativeError in favor of ErrorisError #58262
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
base: main
Are you sure you want to change the base?
doc: deprecate utilsNativeError in favor of ErrorisError #58262
Conversation
Also to keep in mind:
I'm seeing a >50% performance regression for
I don't think that deprecation should really be considered unless this gets addressed upstream.
As a side note, Originally posted by @Renegade334 in #57962 (comment) |
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
I still find it a premature decision to deprecate this method in favour of one which offers the same functionality, but at present takes 2.5x as long to execute – this does not represent equivalence imho. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the link updated to point to MDN instead of the proposal
bdc175c
to
805b32f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still find it a premature decision to deprecate this method in favour of one which offers the same functionality, but at present takes 2.5x as long to execute – this does not represent equivalence imho.
I tend to agree with this. Also there are unresolved issues1 with Error.isError
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still find it a premature decision to deprecate this method in favour of one which offers the same functionality, but at present takes 2.5x as long to execute – this does not represent equivalence imho.
Just for the record, the speed difference seems to depend on the value. I'm getting the following numbers from 1e8 runs:
value | isNativeError | isError |
---|---|---|
new Error() | 1199.98ms | 2618.24ms |
new TypeError() | 1902.15ms | 2547.55ms |
new DOMException() | 1829.65ms | 2942.06ms |
new (class extends Error {})() | 1900.60ms | 2552.22ms |
Object.create(Error.prototype) | 1829.65ms | 2941.48ms |
{} | 1829.62ms | 2942.25ms |
[] | 1829.03ms | 2942.11ms |
42 | 1685.01ms | 2439.98ms |
'error' | 1829.61ms | 3274.12ms |
null | 1895.54ms | 3316.59ms |
undefined | 1968.88ms | 3187.24ms |
The difference for new Error()
is indeed high, there probably should be a fast path implemented on v8 side.
Nevertheless, I don't see it as blocker for the doc-only deprecation. It might be a blocker for next steps, assuming that the situation won't change. In the worst case scenario (if it would be impossible to optimize in v8), we can keep isNativeError()
as legacy solely for performance reasons.
Also for performance reasons, we can keep isNativeError()
for internal use in Node.js core as long as it's faster than ErrorIsError()
.
In the scope of this PR, it might be worth adding a note in the docs that Error.isError()
is currently slower.
Also there are unresolved issues1 with Error.isError
This is not really an issue with Error.isError
itself, it's more of an issue with DOMException
implementation in Node.js. util.types.isNativeError(new DOMException(1))
also returns false
, and fix to this issue is likely to be applicable to both functions.
Sorry, Livia already mentioned this. |
I think this is premature and entirely unnecessary but not to a point where i would needlessly block this over a subjective opinion.
Refs #57962
We now have Error.isError available and it can be an alternative for
util.types.utilisNativeError