Linting

[goosewobbler]

Follow-up to #1310 with reduced scope and improved config, cutting the number of issues by ~75%. Most issues to fix are things like unused vars. I believe this is a nice balance between fixing low-risk, low-hanging fruit and having a good base to build on - as we rewrite bits of Frame and convert more of the codebase to TS we can gradually enable more rules, remove exemptions and generally tighten things up.

Enabling ESLint in VS Code

Install the ESLint extension, switch it to the pre-release version (they are slow with official releases it seems) and add the following config to your .vscode/settings.json in the Frame repo you want to lint:

  "eslint.options": {
    "overrideConfigFile": "eslint.config.mjs"
  }

These config items shouldn't be required when ESLint v9 is released and when Eslint supports eslint.config.mjs, see the following issues for context:
eslint/eslint#16580
microsoft/vscode-eslint#1518

Config & Plugins

As per previous PR, but reduced.

@typescript-eslint/eslint-plugin - flags TS issues, especially around type safety (basic config used for now)
eslint-plugin-jest - flags antipatterns in our tests (rules disabled for now)
eslint-plugin-react - flags antipatterns in React usage
eslint-plugin-react-hooks - flags misuse of hooks in React
eslint-plugin-testing-library - flags issue & provides tips around testing-library usage - all issues fixed by #1334

Future work (tickets to be added to maintenance scope)

• Enable @typescript-eslint/recommended-requiring-type-checking (address issues - refactoring likely, requires stricter use of TS)
• Enable eslint-plugin-react & eslint-plugin-react-hooks for TSX (when we start using TS in renderer)
• Evaluate for potential use:
  • eslint-plugin-import (imports - blocked by flat config support - tracked in [Feature Request] Support new ESLint flat config import-js/eslint-plugin-import#2556)
  • eslint-plugin-comments (keeps track of eslint comment usage - abandoned, eventually will move to eslint-community org)
  • eslint-plugin-n (node - lifeboat from abandoned eslint-plugin-node, eventually...maybe another year...will be unforked)
  • eslint-plugin-promise (promises)
  • eslint-plugin-jsx-a11y (jsx accessibility)
  • eslint-plugin-security (node security)
  • eslint-plugin-markdown (markdown, useful if we end up expanding our repo docs)
  • eslint-plugin-sonarjs (bugs & suspicious patterns)
  • eslint-plugin-unicorn (misc rules)

TODO (* can be punted to a follow-up in order to reduce PR size)

• Merge component tests refactor #1334, rebase
• Convert eslint config to new file format (.eslintrc support will be removed in Eslint v9)
• * Merge Linting fixes #1470
• * Merge {enable jest rules, add fixes PR}
• * Simplify config through use of .jsx extension in all files using it - Parcel should ensure this just works (tm)
• * Add and verify pre-commit hook

[mholtzman]

@goosewobbler think this looks good. just to be clear, this just puts the rules in place but doesnt run them or enforce anything yet?

[goosewobbler]

@mholtzman that's correct. The original plan was to merge fixes into this branch but I think smaller PRs is better. I just removed the pre-commit hook so we can merge this and see the output on running the lint script. The config changes a fair bit in #1346 but that's fine and I think that PR is close too, just need to resolve the noop and the other remaining issues which I listed here. We can then follow up with the other items in the TODO above as separate PRs, one by one. The rules will be enforced when we enable the pre-commit hook.

[socket-security]

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Ignoring: core-js@3.29.0

Powered by socket.dev

[goosewobbler]

@SocketSecurity ignore core-js@3.29.0

[mholtzman]

this lgtm, just a couple minor nits

[mholtzman]

do we want process to be available globally in all browser type files? seems like this might be a good case for one-of exceptions in individual files if we have any

[goosewobbler]

I think it's available everywhere in the renderer process though
https://www.electronjs.org/docs/latest/api/process#sandbox

[mholtzman]

this isn't a global, it's Electron's process object which gets imported from the Electron library like import { process } from 'electron'

[goosewobbler]

I was sure that some renderer files have a global, anyway I'll try the exception route and put what I find here

[goosewobbler]

Ok, here are the renderer files where we're accessing a global process:

frame/app/dash/index.js

Line 15 in 9250bbf

if (process.env.NODE_ENV !== 'development') {

frame/app/onboard/index.js

Line 15 in 9250bbf

if (process.env.NODE_ENV !== 'development') {

frame/app/tray/index.js

Line 15 in 9250bbf

if (process.env.NODE_ENV !== 'development') {

frame/app/dapp/index.js

Line 15 in 9250bbf

if (process.env.NODE_ENV !== 'development') {

Entry points basically - I'll add an entry for these as the global isn't available any further up.

[mholtzman]

we must be doing something to inject this value somewhere, though I'm not sure where. when I try to print process.env to the console it shows undefined, but process.env.NODE_ENV shows "development". are we somehow mapping this?

[mholtzman]

think it's due to bullet point #4 here: https://en.parceljs.org/env.html#%F0%9F%8C%B3-environment-variables