util: update isError with new Error.isError

[miguelmarcondesf]

Refs 57961
Fixes: #57961

This PR updates the isError function in lib/internal/util.js to use the native Error.isError method if it is available.

[miguelmarcondesf]

Do we need to ensure backward compatibility?

[ljharb]

Error.isError should be the entirety of what isNativeError does, and isNativeError should no longer be needed nor instanceof used.

Wouldn't it make more sense to just remove isError entirely, and use the ErrorIsError primordial directly, and add "don't land" labels on this PR?

[geeksilva97]

+1, @ljharb

[miguelmarcondesf]

Got it! Do you think it is okay to continue with this PR and issue or would it be better to create a new one?

[ljharb]

seems fine to me to adapt this PR to do that.

[BridgeAR]

We still want to match errors that are not regular ones. The wild has many examples of these, so we should keep instanceof.

[fisker]

We should deprecate .isError like .isArray

[LiviaMedeiros]

Just for some clarification:
We have two related functions, util.types.isNativeError() and internalUtil.isError().

The former is exposed to userspace, and is going to be superseded by Error.isError().
Internally we can replace it with ErrorIsError primordial, without fallback and without landing on previous release lines. The only problem is that until V8 update lands on main, there will be no such primordial hence the patched code won't work and pass the CI.
We also can deprecate (starting from doc-only deprecation) and eventually remove this utility. Deprecation before the alternative gets released might look questionable but it's not a hard blocker.

The latter is an internal utility that also checks instanceof Error which can be true even if Error.isError() is not (and vice versa).
Internally we can change or even remove it as long as there's no breaking changes to userspace (or, if there are breaking changes and they are justified, we can do it as semver-major change), but it's likely that simply omitting instanceof check would cause a lot of breakage.

[ljharb]

If we have any internal errors that are instanceof Error but not proper errors (such that Error.isError returns false on them) then that's something we should fix asap.

[LiviaMedeiros]

I believe internalUtil.isError() is used only in assert, inspect, repl, and test_runner. All of these have reasons to expect non-errors with Error in prototype chain sent from wild userspace, and internalUtil.isError() helps with covering this case.
If not, it should be fixed indeed.

[miguelmarcondesf]

Just for some clarification: We have two related functions, util.types.isNativeError() and internalUtil.isError().

The former is exposed to userspace, and is going to be superseded by Error.isError(). Internally we can replace it with ErrorIsError primordial, without fallback and without landing on previous release lines. The only problem is that until V8 update lands on main, there will be no such primordial hence the patched code won't work and pass the CI. We also can deprecate (starting from doc-only deprecation) and eventually remove this utility. Deprecation before the alternative gets released might look questionable but it's not a hard blocker.

The latter is an internal utility that also checks instanceof Error which can be true even if Error.isError() is not (and vice versa). Internally we can change or even remove it as long as there's no breaking changes to userspace (or, if there are breaking changes and they are justified, we can do it as semver-major change), but it's likely that simply omitting instanceof check would cause a lot of breakage.

They mentioned in the issue that util.isError is in fact EOL
https://nodejs.org/api/deprecations.html#DEP0048

[miguelmarcondesf]

I believe internalUtil.isError() is used only in assert, inspect, repl, and test_runner. All of these have reasons to expect non-errors with Error in prototype chain sent from wild userspace, and internalUtil.isError() helps with covering this case. If not, it should be fixed indeed.

It seems to be also used in comparisons

[miguelmarcondesf]

So given all the context, do you think it would be best to just update the doc to use Error.IsError (and not the reference to use Object.prototype.toString(arg) === '[object Error]' || arg instanceof Error) and wait for the new version to arrive to make it easier to find out if there are any cases outside of those mentioned that we expect to be non-errors?

[LiviaMedeiros]

They mentioned in the issue that util.isError is in fact EOL
https://nodejs.org/api/deprecations.html#DEP0048

util.isError() was part of the public API, which is indeed no longer the case. internalUtil.isError() is an internal helper function that should still exist.
As soon as Error.isError() becomes a thing, we can adjust the implementation of internalUtil.isError() by replacing isNativeError() with ErrorIsError().

So given all the context, do you think it would be best to just update the doc to use Error.IsError (and not the reference to use Object.prototype.toString(arg) === '[object Error]' || arg instanceof Error) and wait for the new version to arrive to make it easier to find out if there are any cases outside of those mentioned that we expect to be non-errors?

We don't need to reference || arg instanceof Error in the docs at all, since we aren't exposing this logic to userland.

I think the best path forward for this PR is to wait for the V8 update to land on main, rebase on it, and then replace all occurrences of isNativeError() with ErrorIsError(). This change would be purely internal: no documentation changes or further justification needed, provided nothing breaks.

Independently, a doc-only deprecation PR for util.types.isNativeError() can be opened.
If you open it before Error.isError() is available, you might need to convince any opponents that deprecating without an alternative is a good idea, and that alternative will behave exactly as isNativeError() does.
Otherwise the deprecation process should be pretty straightforward.

[miguelmarcondesf]

Thanks @LiviaMedeiros for all the clarification!

I agree that we should wait for v8 and I'll make the adjustment to all isNativeErrors and I'll also wait to open the issue related to its depreciation, it seemed simpler to me.

[Renegade334]

Independently, a doc-only deprecation PR for util.types.isNativeError() can be opened.

I'm seeing a >50% performance regression for Error.isError over util.types.isNativeError in the current RC.

custom/type-check-nativeerror.js n=1000000 handler="error-iserror" type="native-error": 55,167,312.528783545
custom/type-check-nativeerror.js n=1000000 handler="isnativeerror" type="native-error": 127,300,251.90173846
custom/type-check-nativeerror.js n=1000000 handler="error-iserror" type="native-error-crossrealm": 50,549,863.72514987
custom/type-check-nativeerror.js n=1000000 handler="isnativeerror" type="native-error-crossrealm": 123,061,852.73311144
custom/type-check-nativeerror.js n=1000000 handler="error-iserror" type="non-native-error": 53,863,525.66184942
custom/type-check-nativeerror.js n=1000000 handler="isnativeerror" type="non-native-error": 117,042,527.28524657

I don't think that deprecation should really be considered unless this gets addressed upstream.

As soon as Error.isError() becomes a thing, we can adjust the implementation of internalUtil.isError() by replacing isNativeError() with ErrorIsError().

As a side note, ErrorIsError will not be exposed as a primordial while it remains a flagged feature in V8.

[ljharb]

This PR can't land until it's unflagged, regardless.

[miguelmarcondesf]

Hey, I'm doing some local tests, and indeed, we can't change our internal.isError to use Error.isError because, like @LiviaMedeiros mentioned, we expect some non-errors, and it starts to break a lot of internals like assert. Besides this, I'm not seeing any use of isNativeError outside that context.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.06%. Comparing base (5f252a4) to head (1f56578).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/util.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57962      +/-   ##
==========================================
- Coverage   90.12%   90.06%   -0.06%     
==========================================
  Files         629      629              
  Lines      186622   186638      +16     
  Branches    36624    36568      -56     
==========================================
- Hits       168186   168093      -93     
- Misses      11217    11349     +132     
+ Partials     7219     7196      -23     

Files with missing lines	Coverage Δ
lib/internal/util.js	95.70% <0.00%> (-0.60%)	⬇️

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[miguelmarcondesf]

Okay, so keeping e instanceof Error and replacing isNativeError with ErrorisError seems right!
Also, I found other places to replace it, and it seems to be fine as well.

I'll also open another PR to make the deprecation of isNativeError.

[miguelmarcondesf]

Independently, a doc-only deprecation PR for util.types.isNativeError() can be opened.

I'm seeing a >50% performance regression for Error.isError over util.types.isNativeError in the current RC.

custom/type-check-nativeerror.js n=1000000 handler="error-iserror" type="native-error": 55,167,312.528783545
custom/type-check-nativeerror.js n=1000000 handler="isnativeerror" type="native-error": 127,300,251.90173846
custom/type-check-nativeerror.js n=1000000 handler="error-iserror" type="native-error-crossrealm": 50,549,863.72514987
custom/type-check-nativeerror.js n=1000000 handler="isnativeerror" type="native-error-crossrealm": 123,061,852.73311144
custom/type-check-nativeerror.js n=1000000 handler="error-iserror" type="non-native-error": 53,863,525.66184942
custom/type-check-nativeerror.js n=1000000 handler="isnativeerror" type="non-native-error": 117,042,527.28524657

I don't think that deprecation should really be considered unless this gets addressed upstream.

As soon as Error.isError() becomes a thing, we can adjust the implementation of internalUtil.isError() by replacing isNativeError() with ErrorIsError().

As a side note, ErrorIsError will not be exposed as a primordial while it remains a flagged feature in V8.

Hey @Renegade334 sorry but what do you mean by "flagged feature in V8"?

[jasnell]

... sorry but what do you mean by "flagged feature in V8"?

The Error.isError API, while enabled by default now in Node 24.0.0 is still only conditionally enabled in v8 using a flag (js-error-iserror), putting it in a special category. When Node.js builds the primordials object the Error.isError(...) function function not actually exist yet on the Error object. It might be a while before it is Just Available without the flag.

[miguelmarcondesf]

... sorry but what do you mean by "flagged feature in V8"?

The Error.isError API, while enabled by default now in Node 24.0.0 is still only conditionally enabled in v8 using a flag (js-error-iserror), putting it in a special category. When Node.js builds the primordials object the Error.isError(...) function function not actually exist yet on the Error object. It might be a while before it is Just Available without the flag.

Thank you for that explanation!
So given this and the possible performance issues, do you think it's best to close these changes for now or wait a little longer?

[geeksilva97]

... sorry but what do you mean by "flagged feature in V8"?

The Error.isError API, while enabled by default now in Node 24.0.0 is still only conditionally enabled in v8 using a flag (js-error-iserror), putting it in a special category. When Node.js builds the primordials object the Error.isError(...) function function not actually exist yet on the Error object. It might be a while before it is Just Available without the flag.

Thank you for that explanation! So given this and the possible performance issues, do you think it's best to close these changes for now or wait a little longer?

We can keep it with a blocked tag since we need to wait for this unflagging