Skip to content

Conversation

@jmccand
Copy link
Collaborator

@jmccand jmccand commented Nov 24, 2025

Describe what you are trying to do
Fix the misconfigured ESLint and fix all current errors in ESLint, and make it a GitHub Action that runs on push and pull, so that all frontend code will be linted.

Steps for review

  1. Try changing something (removing a type, removing dependencies in useEffect, ...).
  2. npm run lint.

Copy link
Contributor

@williamschen23 williamschen23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'd reconsider some of the missing dependency arrays. Some of them do make sense excluding, which is why personally id rather disable the eslint rule (or warning) over trying to fix them

};
mapkitScript();
}, []);
}, [token]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since token never changes, we shouldnt useMemo it. In react 19.2, there is an useEventEffect function that seems to be perfect for this. In addition, we don't need to fix all the dependency array issues since they are warnings and not errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change. I pass --max-warnings=0, so it doesn't tolerate warnings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint github actions doesnt fail on warnings i thought?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by default it doesn't, but if you pass --max-warnings it will.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to enforce 0 warnings? I feel like sometimes the warnings are fine as react dependency arrays don't really need some of the dependencies eslint is hinting at

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants