Linting by goosewobbler · Pull Request #1337 · floating/frame

goosewobbler · 2023-01-12T00:09:08Z

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

package.json

mholtzman · 2023-01-16T14:55:17Z

@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 · 2023-01-16T15:05:17Z

@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 · 2023-03-01T17:41:13Z

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 [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Ignoring: [email protected]

Powered by socket.dev

goosewobbler · 2023-03-01T19:45:44Z

@SocketSecurity ignore [email protected]

mholtzman

this lgtm, just a couple minor nits

eslint.config.mjs

mholtzman · 2023-03-06T04:40:13Z

eslint.config.mjs

+    languageOptions: {
+      globals: {
+        ...globals.browser,
+        process: true,


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

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

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

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

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.

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?

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

Co-authored-by: goosewobbler <[email protected]>

goosewobbler added WIP PRs that are still in progress and not ready for review or merging tooling maintenance labels Jan 12, 2023

goosewobbler mentioned this pull request Jan 12, 2023

Linting #1310

Closed

goosewobbler force-pushed the linting-2 branch from 781032c to 4b70a76 Compare January 13, 2023 13:35

goosewobbler added the dependencies Pull requests that update a dependency file label Jan 13, 2023

goosewobbler force-pushed the linting-2 branch from 8a60d1a to 7b2ae6b Compare January 14, 2023 14:26

goosewobbler added the linting label Jan 14, 2023

goosewobbler force-pushed the linting-2 branch from d27c817 to e760a1a Compare January 14, 2023 21:08

Repository owner deleted a comment from socket-security bot Jan 15, 2023

mholtzman reviewed Jan 16, 2023

View reviewed changes

package.json Show resolved Hide resolved

mholtzman requested a review from floating January 16, 2023 14:52

goosewobbler removed the WIP PRs that are still in progress and not ready for review or merging label Jan 16, 2023

goosewobbler added this to the post-omnichain release milestone Jan 17, 2023

goosewobbler and others added 5 commits March 1, 2023 16:56

non-package changes

ff78b35

use build tsconfig

43e3176

update

7a96e6b

Update .lintstagedrc.json

34b4080

update ur lock

f9ed60f

goosewobbler force-pushed the linting-2 branch from 6c477c9 to f9ed60f Compare March 1, 2023 17:38

goosewobbler and others added 3 commits March 1, 2023 18:13

update config

1e7024a

Update eslint.config.mjs

45d2580

Update eslint.config.mjs

47682bf

Update package.json

fa17aff

goosewobbler mentioned this pull request Mar 1, 2023

Linting fixes #1470

Merged

Update tsconfig.json

605ab12

Update tsconfig.build.json

ef3e1ed

mholtzman reviewed Mar 6, 2023

View reviewed changes

goosewobbler and others added 4 commits March 6, 2023 18:34

address PR comments

a47488c

add more files, exempt e2es

397c2f7

remove process global for renderer

694eb71

add renderer entry points

41b51cc

mholtzman approved these changes Mar 9, 2023

View reviewed changes

goosewobbler merged commit 5858973 into develop Mar 9, 2023

goosewobbler deleted the linting-2 branch March 9, 2023 12:42

mholtzman pushed a commit that referenced this pull request Mar 21, 2023

Linting (#1337)

7ae3ed2

Co-authored-by: goosewobbler <[email protected]>

Conversation

goosewobbler commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Enabling ESLint in VS Code

Config & Plugins

Future work (tickets to be added to maintenance scope)

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

Uh oh!

Uh oh!

mholtzman commented Jan 16, 2023

Uh oh!

goosewobbler commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socket-security bot commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Socket Security Pull Request Report

Uh oh!

goosewobbler commented Mar 1, 2023

Uh oh!

mholtzman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mholtzman Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

goosewobbler Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

mholtzman Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

goosewobbler Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

goosewobbler Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mholtzman Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

mholtzman Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

goosewobbler commented Jan 12, 2023 •

edited

Loading

goosewobbler commented Jan 16, 2023 •

edited

Loading

socket-security bot commented Mar 1, 2023 •

edited

Loading

goosewobbler Mar 8, 2023 •

edited

Loading