Linting fixes (main process & scripts)#1346
Conversation
8a60d1a to
7b2ae6b
Compare
acf3db8 to
dde331c
Compare
app/dapp/index.js
Outdated
| @@ -1,5 +1,5 @@ | |||
| /* globals global, process */ | |||
There was a problem hiding this comment.
is there a way to define globals, ahem, globally?
There was a problem hiding this comment.
There is, we're using the globals package for this.
https://github.com/floating/frame/pull/1346/files#diff-9601a8f6c734c2001be34a2361f76946d19a39a709b5e8c624a2a5a0aade05f2R55
https://github.com/sindresorhus/globals/blob/main/globals.json
However I thought this file is renderer process only -- 'global' (as opposed to 'globalThis') and 'process' are only available in the node environment, we are only setting globals.browser (see link above). I logged them out and verified that they do exist at runtime, though. Basically I got confused and decided to exempt them here...
EDIT:
Seems 'global' and 'process' are both available in both renderer and main processes. I think the electron docs are a bit hazy on this, couldn't find any info on 'global' and it's really not obvious where 'process' is available and what you have access to. I tried changing the use of 'process' to the imported object from electron and it broke everything. globals doesn't have any config for electron, but I added these objects to our eslint config and removed the /* globals */ comments.
https://www.geeksforgeeks.org/process-object-in-electronjs/
https://iamveryhungry.medium.com/share-data-using-global-d141cfc21b7d
eslint.config.mjs
Outdated
| 'error', | ||
| { ignoreRestSiblings: true, argsIgnorePattern: '^_', destructuredArrayIgnorePattern: '^_' } | ||
| ], | ||
| '@typescript-eslint/no-empty-function': 'off', // allow noop functions |
There was a problem hiding this comment.
do we have a lot of functions that require this carve out? would prefer to keep it on if possible
There was a problem hiding this comment.
Current totals in this branch at time of typing:
Rule on: 254 problems (173 errors, 81 warnings)
Rule off: 248 problems (167 errors, 81 warnings)
So 6 instances. I would rather keep it on too, could have a single exemption in a resources file and any file where we're using an empty func imports that? I'm also trying to reduce refactoring before the omni release.
e.g. import { noop } from 'resources/utils'
There was a problem hiding this comment.
We can still make the above change and disable it completely (with one exemption as mentioned above) but as an intermediary I restricted the rule to arrow functions only.
eslint.config.mjs
Outdated
| 'no-undef': 'off', // redundant - TS will fail to compile with undefined vars | ||
| '@typescript-eslint/no-unused-vars': [ | ||
| 'error', | ||
| { ignoreRestSiblings: true, argsIgnorePattern: '^_', destructuredArrayIgnorePattern: '^_' } |
There was a problem hiding this comment.
are there cases where we want to keep the _var format for specific vars? I'd prefer to use the after-used option here personally: https://eslint.org/docs/latest/rules/no-unused-vars#args
There was a problem hiding this comment.
Personally I'm not a fan of comma dangle outside of this use case as it's usually just a crutch to avoid having to name things properly or even more lazily, to avoid having to restrict access to methods / properties which should be private. Note that I didn't specify varsIgnorePattern here. Updated to use after-used
| @@ -1,5 +1,3 @@ | |||
| import React from 'react' | |||
There was a problem hiding this comment.
for my own information, what rule causes us to remove these React import statements from lots of files, why aren't they needed, and were they at some point in the past?
There was a problem hiding this comment.
These were previously required because JSX required a React import to be in scope in order to use it. React 17 changed that with the new JSX transform.
https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html#removing-unused-react-imports
https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/react-in-jsx-scope.md
|
@goosewobbler looks like some tests are broken |
d27c817 to
e760a1a
Compare
e157b8f to
e580e3b
Compare
|
@mholtzman there were shenanigans with ledger tests breaking on some intra lib import. I think it was some weird npm / lockfile behaviour. Should be fixed now, otherwise will need to recreate with cherrypick |
|
different error, another odd one. Will address this and the rest of the comments tomorrow / monday |
|
Remaining errors to address: Don't use
frame/main/transaction/actions/index.ts Line 11 in a8bdc8e Do not use "@ts-ignore" because it alters compilation errors. Line 5 in a8bdc8e Line 10 in a8bdc8e Promise executor functions should not be async. Don't use frame/main/externalData/balances/index.ts Line 44 in a8bdc8e 'task' is not defined. Line 5 in a8bdc8e |
003e82d to
96be519
Compare
|
Closing in favour of #1470 |
Addressing all Main process and script file linting issues.
Note we are not addressing warnings at the moment - there are a lot of
anytypes which we absolutely should replace as our use of TS in Frame expands and improves, but addressing this now requires a fair bit of work and also high levels of risk due to the amount of refactoring involved.