-
Notifications
You must be signed in to change notification settings - Fork 101
Migrate from eslint formatting to Prettier #3252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Eslint formatting rules are deprecated (see https://eslint.org/blog/2023/10/deprecating-formatting-rules/ ). This commit configures the prettier plugin and moves the formatting rules in the new .prettierrc file.
`prettier --write "**/*.{ts,tsx,js,jsx}"` launched from frontend/web/src/ Please note that `hooks/api.ts` has been slightly modified, to keep the `react-hooks/exhaustive-deps` lint disable active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked everything. But I would like to have a look at https://eslint.style/ see comments.
If we go with prettier this PR should add instructions in the README and a task in npm and Makefile
"@walletconnect/jsonrpc-types": "1.0.4", | ||
"@walletconnect/types": "2.17.2", | ||
"@reown/walletkit": "1.2.0", | ||
"flag-icons": "7.2.3", | ||
"i18next": "23.11.5", | ||
"lightweight-charts": "4.1.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add an npm task below under "scripts": {
something like:
"format": "prettier --write '**/*.{ts,tsx,js,jsx}'",
@@ -2,447 +2,469 @@ | |||
// Copyright (C) 2016 Klarälvdalens Datakonsult AB, a KDAB Group company, [email protected], author Milian Wolff <[email protected]> | |||
// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR LGPL-3.0-only OR GPL-2.0-only OR GPL-3.0-only | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please exclude this file
}; | ||
|
||
export const getAccountsTotalBalance = | ||
(): Promise<TAccountsTotalBalanceResponse> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier has some very aggressive newline changes, I dislike having newline after =
.
If we slightly increase "printWidth": 90
it would keep this, but change in many other places where we want newlines i.e. function arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should go with printWidth 100 imho. It's what I use as my default width also for backend code. The default 80 is like from the 80s 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried 90, 100, 120, but IMO gets even worse with any of those.
I'd like to try eslint.style soon™
accountName: string, | ||
} | ||
) => void, | ||
cb: (meta: { count: number; accountName: string }) => void, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bit: This is a case where I find the aggressive newlines wrapping less readable, but changes overall seem to be an improvement
displayName: string | ||
} | ||
currency: Fiat; | ||
displayName: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
); | ||
export const subscribeCoinHeaders = | ||
(coinCode: CoinCode) => (cb: TSubscriptionCallback<TStatus>) => | ||
subscribeEndpoint(`coins/${coinCode}/headers/status`, cb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned before I dislike newlines after =
but also after =>
maybe there is still a way in the future to improve or trick prettier by using = ( /* fn goes here */);
@@ -140,8 +145,8 @@ export const App = () => { | |||
|
|||
// If a device is newly connected, we route to the settings. | |||
if ( | |||
newDeviceIDList.length > 0 | |||
&& newDeviceIDList[0] !== oldDeviceIDList[0] | |||
newDeviceIDList.length > 0 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dislike having the condition at the end.
geebeetee suggested operator-linebreak rule to fix, but seems that this is deprecated
https://eslint.org/docs/latest/rules/operator-linebreak
interestingly they suggest using @stylistic/eslint-plugin-js.
This rule was deprecated in ESLint v8.53.0. Please use the corresponding rule in @stylistic/eslint-plugin-js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth, afaik it's much more common to have the operators at the end of the line from the code I am seeing across many languages.
Imho not worth changing away from the prettier default here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just my personal preference and helps me reading the code. I didn't say it's more common, but it is what we mostly use in our codebase.
); | ||
} | ||
return null; | ||
})} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍
{' '} | ||
{conversionUnit} | ||
</span> | ||
<span className={styles.txUnit}> {conversionUnit}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever we add {' '}
there is a reason, I dislike that it just drops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they are equivalent, except for if conversionUnit
has leading spaces, which would then collapse to one. I guess this is not the case here.
Or can you think of differences between a regular space and {' '}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deepseek:
**Visual Effect:**
- Literal space becomes a normal space
- `{' '}` becomes ` ` (HTML entity for space)
- Both will render a single space before the `conversionUnit` value
- Visual difference only occurs in edge cases:
- If parent element has `white-space: pre/pre-line`
- If using CSS `::before`/`::after` pseudo-elements
- If the conversionUnit value contains leading/trailing spaces
so I guess they are not always the same if the above is true. But then it's strange that prettier would default to this transformation if it is not equivalent 🤔
Maybe we can ignore locally (// prettier-ignore
?) for when it actually matters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space in JSX and {' '}
should behave the same.
When I read the code I want to see {' '}
so I know it was explicitly added.
Here is a recap of the rules changes in the eslint -> prettier migration
📘 Full ESLint Formatting Rules vs. Prettier
brace-style: ['error', '1tbs']
comma-spacing: ['error', { before: false, after: true }]
indent: ['error', 2]
tabWidth: 2
.prettierrc
jsx-quotes: ['error', 'prefer-double']
jsxSingleQuote: false
.prettierrc
keyword-spacing: 'error'
if
,else
, etc.no-multi-spaces: 'error'
no-trailing-spaces: 'error'
object-curly-spacing: ['error', 'always']
bracketSpacing: true
.prettierrc
quotes: ['error', 'single']
singleQuote: true
.prettierrc
semi: 'error'
semi: true
.prettierrc
space-before-blocks: ['error', 'always']
{
space-in-parens: ['error', 'never']
()
no-extra-semi: 'error'
@typescript-eslint/type-annotation-spacing: 'error'
arrow-spacing: 'error'
arrowParens: 'always'
=>
space-infix-ops: 'error'
=
,+
, etc.react/jsx-equals-spacing: ['error', 'never']
=
in JSX propsreact/jsx-wrap-multilines: {...}
()
by default, not configurableBefore asking for reviews, here is a check list of the most common things you might need to consider: