Skip to content
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

add eslint back in to setup app #13

Merged
merged 3 commits into from
Jan 7, 2025
Merged

add eslint back in to setup app #13

merged 3 commits into from
Jan 7, 2025

Conversation

kswanson33
Copy link
Collaborator

@kswanson33 kswanson33 commented Dec 23, 2024

I removed some linting in #12, since next linting breaks if files we want to 🔥 aren't there. But eslint is still helpful, especially for removing unused imports.

@kswanson33 kswanson33 changed the title add eslint back in to linting setup app add eslint back in to setup app Dec 23, 2024
@kswanson33 kswanson33 requested a review from a team December 23, 2024 15:54
Copy link

@philschatz philschatz left a comment

Choose a reason for hiding this comment

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

👍

.eslintrc.json Outdated
@@ -1,11 +1,13 @@
{
"extends": "next/core-web-vitals",
Copy link
Contributor

@chrisbendel chrisbendel Dec 23, 2024

Choose a reason for hiding this comment

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

i wonder if since next is complaining certain files don't exist, maybe this is why? i wonder what happens if we switch to some other base config we wouldn't have to worry about it

some discussion here

and maybe some relevant docs here

i could be overthinking this though 🤔

Copy link
Collaborator Author

@kswanson33 kswanson33 Dec 23, 2024

Choose a reason for hiding this comment

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

good catch! I got the error re: next when running next lint (removed here) -- but not when running eslint, even with this setting. But I agree this isn't the best base thing for us to use.

I tried removing the extends field as well as replacing the above line with "extends": "eslint:recommended". Everything seems to work the same, except weirdly, I'm getting the below error in sorter.js

image

Note that eslint is even disabled above 😱 . Going to keep looking into this ...

Copy link
Contributor

Choose a reason for hiding this comment

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

woah 😮 that is so weird!!

Copy link
Member

Choose a reason for hiding this comment

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

heads up that nextjs 15 upgrades eslint to the current version and it's config file format changes. management app change to that is here: https://github.com/safeinsights/management-app/pull/48/files#diff-9601a8f6c734c2001be34a2361f76946d19a39a709b5e8c624a2a5a0aade05f2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, i removed next/core-web-vitals & fixed the above issue by adding config to eslintrc 311d2ec

then the CLI broke in a new weird way (only on Github Actions, worked locally fine). so I updated eslint per nathan's recommendation & migrated eslintrc to eslint.config.mjs using the auto-migrator. now everything works again 🤞...

so that was cool and easy and everything worked as expected

Copy link
Collaborator Author

@kswanson33 kswanson33 Dec 23, 2024

Choose a reason for hiding this comment

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

i mean -- the current config SHOULD be fine for this project. i just feel like some of these other buggy configs should also have been fine. but whatever 🤷

@kswanson33 kswanson33 merged commit 2951b6a into main Jan 7, 2025
1 check passed
@kswanson33 kswanson33 deleted the kc-linting branch January 7, 2025 15:29
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.

4 participants