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

Simplify ESLint Config #261

Merged
merged 8 commits into from
Oct 12, 2023
Merged

Simplify ESLint Config #261

merged 8 commits into from
Oct 12, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Oct 3, 2023

This is the first step towards deprecating the @remix-run/eslint-config package as outlined in remix-run/remix#7056.

Remix doesn't need to be in the business of providing opinions on how teams lint their Remix app code - that should be a team decision. The stacks can and should provide a working ESLint setup that teams can tweak to their liking. So this PR removes the @remix-run/eslint-config dependency and simplifies the config to a single-file that just relies on industry "recommended" plugins for technologies used by the stacks (react, TS, vitest, cypress, etc.).

@brophdawg11 brophdawg11 changed the title Simplify Simplify ESLint Config Oct 3, 2023
Comment on lines +47 to +52
rules: {
"react/jsx-no-leaked-render": [
"warn",
{ validStrategies: ["ternary"] },
],
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could go either way on this one-off rule. On one hand, if we truly want to stay out of the opinion game, then we technically should have zero rules or our own. But this one seems like it solves a common footgun, and TBH I was surprised it wasn't enable din the recommended plugin...so I added it. (It was also one we turned on via eslintrc.repo.js before.

Comment on lines +78 to +87
rules: {
"import/order": [
"error",
{
alphabetize: { caseInsensitive: true, order: "asc" },
groups: ["builtin", "external", "internal", "parent", "sibling"],
"newlines-between": "always",
},
],
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as above. I personally hate lack of consistency when it comes to imports - but again this is veering into "opinion" territory.

@@ -29,7 +29,7 @@ jobs:
run: npm install

- name: 👔 Format
run: npm run format:repo
run: npm run format
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only have one config now so no longer need separate format/format:repo and lint/lint:repo options

app/db.server.ts Outdated
// Hard-code a unique key, so we can look up the client when this module gets re-imported
const prisma = singleton("prisma", () => new PrismaClient());
const prisma = remember("prisma", () => new PrismaClient());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

singleton had some TS lint errors so I removed it in favor of remember

@@ -100,6 +100,7 @@ export default function Join() {
ref={emailRef}
id="email"
required
// eslint-disable-next-line jsx-a11y/no-autofocus
autoFocus={true}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These feel like they're an OK exception to the rule as this form is the sole purpose of the page

@@ -1,6 +1,7 @@
import { faker } from "@faker-js/faker";

declare global {
// eslint-disable-next-line @typescript-eslint/no-namespace
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know the best way to do this otherwise so I just disabled it.

@brophdawg11 brophdawg11 merged commit 521a3cb into main Oct 12, 2023
6 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/simplify-eslint branch October 12, 2023 15:32
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.

1 participant