Rule proposal: `prefer-error-iserror`

[papb]

Disallow instanceof Error checks.

Fail

const isError = foo instanceof Error;

Pass

const isError = Object.prototype.toString.call(foo) === '[object Error]'

[fisker]

How about no-instanceof?

[sindresorhus]

Too general. It’s generally fine to instanceof your own types, just not built-ins.

[fisker]

We have Array checked https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/no-array-instanceof.md

[sindresorhus]

Yeah, I think we should just do Error for now. Error is usually the problematic global.

[fisker]

Other built-in errors? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects#Error_objects

[sindresorhus]

Yes, the built-in errors too.

[fregante]

Too general. It’s generally fine to instanceof your own types, just not built-ins.

Is it though? If I load the same library in 2 realms I have the same issue. It's just that built-ins are more common.

instanceof is never safe, it's just easy to use until you have to check something cross-realm.

Example

document instanceof Object
// true

document.querySelector('iframe').contentWindow.document instanceof Object
// false

I think if you want to add this rule it should be a configurable no-instanceof and the default to Error and whatever you encounter the most often.

[sindresorhus]

Sounds good

[papb]

Yes, the built-in errors too.

What would be the suggested fix for foo instanceof TypeError though?

Since Object.prototype.toString.call(new TypeError()) gives '[object Error]', I guess it would have to be

Object.prototype.toString.call(foo) === '[object Error]' && foo.name === 'TypeError'

instead?

[sindresorhus]

Correct

[tinovyatkin]

Depending on a project target, but since Node 10.0 util.types.isNativeError looks like way cleaner approach than Object.prototype.toString.call

[sindresorhus]

Cleaner, yes, but less cross-platform. We cannot know where the user intends to run their code.

[tinovyatkin]

There is an ESLint environment setting for that. https://eslint.org/docs/user-guide/configuring#specifying-environments

"env": {
  "browser": true,
  "node": true,
  "shared-node-browser": true
}

So, actually, yes, the user specified that. And then, there is engines in package.json for Node and .browserslistrc for browsers.

[tinovyatkin]

https://github.com/mysticatea/eslint-plugin-node/blob/master/lib/util/get-configured-node-version.js https://github.com/amilajack/eslint-plugin-compat/blob/master/src/Versioning.js

[tinovyatkin]

Magically smart rule auto fix!

[sindresorhus]

@tinovyatkin Sure, if we can reliably detect that it's node-only environment, we can suggest that one.

[sindresorhus]

This rule is accepted.

[fisker]

Suggest hold this for Error.isError