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.

[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.