Only enable Node lint rules for Node files #3672
Conversation
Does this mean that other files can be incompatible with Node.js? We use some packages in Node.js for Snaps. |
@Mrtenz Oh, sorry for the confusion, I just meant that these rules should only apply to test files and scripts in the context of this monorepo, not as a general statement for all monorepos. For Snaps, it sounds like we'd need some specific configuration to account for Node.js-compatible packages. That would make sense to me. |
The base configuration should ensure things work across Node.js and browsers. We're still applying the base config everywhere, so that should ensure compatibility with Node.js. The Node.js rules are intended for scripts solely run in a Node.js environment, like scripts and tests. |
|
I need to revive this. I'm also unsure we need the |
mcmire
force-pushed
the
enable-nodejs-lint-rules-for-node-files
branch
2 times, most recently
from
1fa7c70
to
65259c9
Compare
|
I've rebased this PR. Ready for a fresh review. |
mcmire
force-pushed
the
enable-nodejs-lint-rules-for-node-files
branch
2 times, most recently
from
5484629
to
7e7a8dd
Compare
mcmire
force-pushed
the
enable-nodejs-lint-rules-for-node-files
branch
from
7e7a8dd
to
bc0e538
Compare
mcmire
force-pushed
the
enable-nodejs-lint-rules-for-node-files
branch
from
bc0e538
to
892e3f5
Compare
|
Rebased again and ready for another re-review. |
In a previous commit, we mistakenly introduced a Node-specific function for testing deep equality, and this ended up crashing the extension. This would usually have been caught by our ESLint rules, as they prohibit use of Node libraries by default. However, the ESLint configuration for this repo imports rules from our `@metamask/eslint-config-nodejs` package and applies them to all files, marking everything as Node-compatible, thereby allowing such usage. This is incorrect: these rules should only apply to test files and scripts. This commit fix the ESLint configuration to match. Doing so revealed a couple of categories of violations, which this commit also fixes: - Some packages import and make use of EventEmitter from Node's `events` module. We add a notice to the README for these packages which advises consumers that these packages are designed to be used in a Node-compatible environment. - Some packages import and make use of Node's `assert` module to check values at runtime. It isn't strictly necessary to use this module, and so we replace its usage with simpler code. - Some packages import and make use of Node's `inspect` utility function. We don't strictly need this either, and we replace its usage with simpler code as well.
mcmire
force-pushed
the
enable-nodejs-lint-rules-for-node-files
branch
from
892e3f5
to
5f1f877
Compare
| `NetworkController state is invalid: \`selectedNetworkClientId\` ${inspect( | ||
| state.selectedNetworkClientId, | ||
| )} does not refer to an RPC endpoint within a network configuration`, | ||
| // False negative - this is a string. |
There was a problem hiding this comment.
Huh. I can reproduce this locally as well. Strange bug.
I don't see any reports of this on the @typescript-eslint repo. Maybe this is caused by us using an incompatible version of TypeScript though.
There was a problem hiding this comment.
Yeah, I'd be curious how the situation is once we upgrade.
|
Fixed conflicts and responded to feedback above. Ready for another review. |
Explanation
In a previous commit, we mistakenly introduced a Node-specific function for testing deep equality, and this ended up crashing the extension. This would usually have been caught by our ESLint rules, as they prohibit use of Node libraries by default. However, the ESLint configuration for this repo imports rules from our
@metamask/eslint-config-nodejspackage and applies them to all files, marking everything as Node-compatible, thereby allowing such usage. This is incorrect: these rules should only apply to test files and scripts.This commit fix the ESLint configuration to match. Doing so revealed a couple of categories of violations, which this commit also fixes:
eventsmodule. We add a notice to the README for these packages which advises consumers that these packages are designed to be used in a Node-compatible environment.assertmodule to check values at runtime. It isn't strictly necessary to use this module, and so we replace its usage with simpler code.inspectutility function. We don't strictly need this either, and we replace its usage with simpler code as well.References
This issue was brought up in our company chat.
Changelog
(No functional changes to note here)
Checklist