-
Notifications
You must be signed in to change notification settings - Fork 3
chore: upgrade eslint configuration #897
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
Conversation
cbf782a
to
4994618
Compare
4994618
to
51e7de1
Compare
1bd4cfb
to
e7e4cc7
Compare
79802e4
to
7bc8761
Compare
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.
Im glad that we are using the new eslint architecture. There are a lot of new rules introduced by the premade/recommended configs. Running eslint
introduced 99 problems (but a bitch aint one 😆 ) - 71 error and 28 warnings.
I think we should using these recommended configs but Im wondering if it should be the job of this PR to solve the new errors associated with it?
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 we should using these recommended configs but Im wondering if it should be the job of this PR to solve the new errors associated with it?
I'm in favor of addressing the issues as follow-ups, mostly because we haven't made an effort to address the issues prior to this PR anyways. I've added the automatic fixes in this PR but if the issues don't fall under that category, I think it's safer to have dedicated PRs to address them.
I'm mostly interested in keeping this PR's scope to updating the tooling and configuration. I think decisions around things like the following are better served as follow-ups:
- deciding which rules to enable/disable
- incorporating CI checks
- addressing the actual issues
@@ -1,7 +1,7 @@ | |||
{ | |||
"name": "mapeo-mobile-node-next", | |||
"version": "1.0.0", | |||
"lockfileVersion": 2, | |||
"lockfileVersion": 3, |
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.
just noting that the massive diff in this file (aside from removing eslint) is due to the lockfile version changing. I think this is desirable in our case, as v3 is associated for npm v9 and later, which aligns with the versions of node that matter to us (18 + 20)
https://docs.npmjs.com/cli/v11/configuring-npm/package-lock-json#lockfileversion
not really sure why the lockfile version changed in this PR versus other PRs where we updated backend deps though 🤔
@@ -2,8 +2,8 @@ import {useEffect, useState} from 'react'; | |||
import {Duration} from 'luxon'; | |||
|
|||
export const useFormattedTimeSince = (start: Date | null, interval: number) => { | |||
const [currentTime, setCurrentTime] = useState(new Date()); | |||
let startDate = start ? new Date(start) : new Date(); | |||
const [currentTime, setCurrentTime] = useState(() => new Date()); |
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.
Which rule complained about this? These are the kind of rules in eslint that bug me a bit. Yes it saves an unnecessary new Date()
on each render, but that is a pico-optimization.
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.
https://eslint-react.xyz/docs/rules/hooks-extra-prefer-use-state-lazy-initialization
yes it's a minor optimization but it's the unfortunate reality of how react works. i'd rather enforce consistency on this rather than relying on everyone to have a deep enough knowledge about this potentially consequential limitation in React
041f24d
to
0096c49
Compare
0096c49
to
6f4b608
Compare
did an initial pass on adjusting rules that are unanimously less helpful and addressing the resulting errors and warnings in 6f4b608. Kind of wish I did a stacked PR so that changes in the app code could be easier to see. happy to move things around if desirable. couple of observations:
while the bugs found so far are relatively minor, nothing stops us from introducing major bugs with these rules being disabled down the line. i'd rather put in the early foundations to mitigate those cases, even if they present some inconveniences in the development experience |
To be discussed tomorrow. I agree with Andrew about the The only one I dont agree with is Otherwise the rest of the rules seem good for me. |
Yeah agreed, i don't find this rule super helpful although I kind of understand the reasoning. Will disable it 👍 |
@react-native/eslint-config
) due to compat issues with the typescript eslint plugin it uses internally.applies very trivial fixes that could be automated by the eslint check (i.e. with itsEDIT: applies fixes to relevant and easy-to-address issues--fix
) flag.Remaining items (can be done as follow-ups):
decide whether or not not having rules specific to React Native for now is a blocker. Can spend some time attempting to get it working but would be okay with doing as a follow-up.EDIT: addressed via ad35fdcadjust the react configuration to omit non-applicable rules (e.g. no need for
dom
)