Revolintion: Apply most of the @thesis-co/eslint-config lint styling to the extension
#3571
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a more restrictive and opinionated set of rules based on the
AirBnB style guide that reduces the degrees of freedom and bikeshedding
across the repo, as well as generally representing Thesis best
practices.
A few notes:
- This definitively moves us from a prior expressed preference for
conditional React rendering of condition ? <Element /> : <></> to the
more common null-punning approach of condition && <Element />. The
style used in the past is disallowed by react/no-jsx-useless-fragment,
which is part of the default airbnb style and therefore inherited
through the Thesis config.
- That rule also shifted a few places where we were returning <></> to
return null instead, and updated return types accordingly. In one or
two places where doing that would have a far-reaching impact, an
eslint-ignore is used for now (with clarifying comment of course),
which can be revisited in the future if we feel strongly about it.
- This also pushes us away from the pattern of having () => {}-style
functions with a single return statement inside, favoring instead
making these return implicitly. A lot of the changes are around that.
- There were also a few cases where undefineds were being left to dangle
that got flagged and now get alternate empty states. Delightful!
Last but not least, one whole rule is disabled so that we can get the
core of this merged: react/no-unstable-nested-components has flagged a
bunch of places where another pattern adopted in the current codebase
(defining anonymous functions that return elements lazily) is being used
in a way that's creating a lot of unnecessary diffs. Probably not
impactful in the large, but regardless it would be good to fix this in
the future, for now the rule is turned off with a FIXME comment.
1 task
4 tasks
|
I would just love a follow-up issue to deal with |
|
mhluongo
approved these changes
Aug 4, 2023
There was a problem hiding this comment.
I manually reviewed all background changes, but could only spot-check ui and various tests. I think it's worth thinking about adding an automated security scan as part of our CI — we're big enough that even PRs from trusted devs deserve more scrutiny.
That said... I'm right next to @Shadowfiend and am confident in how's he's gotten here, so we can kick the can a little bit longer.
| "plugin:prettier/recommended", | ||
| ], | ||
| plugins: ["no-only-tests"], | ||
| extends: ["@thesis-co"], |
| }), | ||
| } | ||
| }, | ||
| inputAmount: (state, { payload }: { payload: string }) => ({ |
There was a problem hiding this comment.
Unfortunate that this still in the codebase. C'est la vie
| @@ -40,7 +40,7 @@ export async function enrichEIP2612SignTypedDataRequest( | |||
| // If we have a corresponding asset - use known decimals to display a human-friendly | |||
| // amount e.g. 10 USDC. Otherwise just display the value e.g. 10000000 | |||
| const formattedValue = asset | |||
| ? `${Number(value) / 10 ** asset?.decimals} ${asset.symbol}` | |||
| ? `${Number(value) / 10 ** (asset?.decimals ?? 0)} ${asset.symbol}` | |||
Merged
Shadowfiend
added a commit
that referenced
this pull request
Aug 9, 2023
Primarily a bug fix release for dApp connections in certain scenarios, particularly stargate.finance . ## What's Changed * v0.45.0 by @jagodarybacka in #3589 * Add `@testing-library/dom` to `background` by @jagodarybacka in #3588 * Modify Hardhat cache config by @michalinacienciala in #3591 * Revolintion: Apply most of the `@thesis-co/eslint-config` lint styling to the extension by @Shadowfiend in #3571 * Release Checklist Update: Added Flashbots by @andreachapman in #3592 * DAppstrosities: Fix a few observed dApp connection issues by @Shadowfiend in #3593 **Full Changelog**: v0.45.0...v0.46.0 Latest build: [extension-builds-3596](https://github.com/tahowallet/extension/suites/14953782899/artifacts/852761295) (as of Wed, 09 Aug 2023 14:34:52 GMT).
Merged
Shadowfiend
added a commit
that referenced
this pull request
Aug 11, 2023
Additional fix for issues on the signing screen from #3571. ## What's Changed (since v0.45.0) * Fix invalid react fragment by @hyphenized in #3597 * v0.46.0 by @Shadowfiend in #3596 * v0.45.0 by @jagodarybacka in #3589 * Add `@testing-library/dom` to `background` by @jagodarybacka in #3588 * Modify Hardhat cache config by @michalinacienciala in #3591 * Revolintion: Apply most of the `@thesis-co/eslint-config` lint styling to the extension by @Shadowfiend in #3571 * Release Checklist Update: Added Flashbots by @andreachapman in #3592 * DAppstrosities: Fix a few observed dApp connection issues by @Shadowfiend in #3593 **Full Changelog**: v0.45.0...v0.46.0 Latest build: [extension-builds-3598](https://github.com/tahowallet/extension/suites/14964619550/artifacts/853742841) (as of Wed, 09 Aug 2023 22:44:33 GMT).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few notes here:
condition ? <Element /> : <></>to the more common null-punning approach ofcondition && <Element />. The style I've suggested in the past is disallowed byreact/no-jsx-useless-fragment, which is part of the default airbnb style and therefore inherited through the Thesis config. Obviously I don't love it, but the point of these is to eliminate relatively pointless debate, so I'm not debating 😁<></>to returnnullinstead, and updated return types accordingly. In one or two places where doing that would have a very far-reaching impact, I've dropped an eslint-ignore for now (with clarifying comment ofc), which can be revisited in the future if we feel strongly about it.() => {}-style functions with a single return statement inside, favoring instead making these return implicitly. A lot of the changes are around that.Last but not least, I've disabled one whole rule so that we can get the core of this merged:
react/no-unstable-nested-componentshas flagged a bunch of places where another pattern I think I suggested (defining anonymous functions that return elements lazily) is being used in a way that's creating a lot of unnecessary diffs. Probably not impactful in the large, but regardless it would be good to fix this. It's a big enough hunk of work that I decided to fire this off before fixing it; it'll be the next thing to circle back and snipe.Testing
Pretty much all the changes here are functionally identical, except for:
!!usage in a case where we were already using booleans. Checking NFT pricing and filtering should be enough to prove this is working as intended.SharedDropdowntoggle function/context to avoid some invalidation issues.I suggest any review turn off whitespace in the GitHub diff view as the whitespace changes are fairly noisy.
Latest build: extension-builds-3571 (as of Fri, 04 Aug 2023 19:03:02 GMT).