-
Notifications
You must be signed in to change notification settings - Fork 2.1k
clean up ESLint configuration in examples #4172
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
clean up ESLint configuration in examples #4172
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
@albertothedev is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
mehulkar
left a comment
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'm ok with this if @tknickman is. All the CI tests for example are failing because the output of turbo run build lint is outputing:
+ \x1b[33mNo caches are enabled. You can try "turbo login", "turbo link", or ensuring you are not passing --remote-only to enable caching\x1b[0m (esc)
I'm not sure why this is, but it's probably because the test cases redirect stdout to /dev/null (with > /dev/null), but not stderr and and this is a new warning printed to stderr? Could it be because of the changes here?
I am glad you're happy with it. I want to draw your attention, however, to the deployment failure of the |
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.
Hey, thanks for the PR! The reason we have 3 eslint configs in our examples (a shared config in packages/, a root/ config, and a config in the app itself) is a little bit confusing but I'll try my best to explain it all here.
-
The shared config (packages/eslint-config-custom):
- The reason for this config is two fold. First, it allows easy config sharing between apps and packages, and second, it helps
turboautomatically understand when your cache should be busted. By default,turboautomatically knows about two things, your apps files, and its dependencies. When any of those change, it understands to skip the cache instead of re-using a previous run. By keeping the config in a shared package that is included as a dependency in the apps / packages that need it, turbo understands to bust the cache when the eslint config changes.
- The reason for this config is two fold. First, it allows easy config sharing between apps and packages, and second, it helps
-
The root config:
- The reason for the root config is simply for editor support (vscode with the eslint extension will look for configuration at the root of the repo by default). Overrides and rules should not be added here as they won't be respected by turbo. You can see this by:
- Starting with the basic example
- Add a console statement to an app
- Run turbo lint (passes, because our lint config allows console statements)
- Add
"no-console": "error",to the root config - Run turbo lint (passes, with a cache hit, because it doesn't know about your root config)
Now, if you added the same line to the apps config, or the shared config, this would work as expected (cache miss and failure) because turbo is automatically aware of these.
- The reason for the root config is simply for editor support (vscode with the eslint extension will look for configuration at the root of the repo by default). Overrides and rules should not be added here as they won't be respected by turbo. You can see this by:
-
The app specific config
- The reason for this config is to provide a place for app / package specific config overrides that are turbo aware (as explained above).
Hopefully that makes sense, given this information - maybe you can help us rework the examples and docs to make this simpler to understand?
Thanks!
IntroductionFirst of all, I'm sorry for the late response. I have been busy moving to a new place, setting up a new computer, and being sick for a couple days. I am currently on vacation from work so I have the time to spend reworking this section of the codebase. With that out of the way, there's a lot to get through and we would all benefit if we better described what we're referring to when typing. Your line:
can be confusing because you are referring to files that perform different functions with the same word: "config". For this reason, I have made this glossary that distinguishes between them: GlossaryWhen a word refers to a term specified here, it will be bold.
Example setupsFor now, let's forget about the changes on my PR. This entire reply refers only to the current state of the codebase, unless when specified otherwise. There are a total of 15 examples, excluding
Questions and concerns
Let me know what you think. |
|
I'm thinking that this PR has gotten a bit messy, so once we have agreed on an approach and I have implemented the changes I would like to close this PR, create an issue documenting the situation and our conclusions, and open a new PR. What do you think? |
|
Hey @albertothedev! We finally got around to cleaning up eslint in our repo / examples. We're still making our way through some of the examples, but the most popular (basic, tailwind, kitchen sink and design system) have all been updated. I'm going to close this one, but thanks again for this PR and kicking off the discussion! |
|
Hi @tknickman. Great to hear! As I said on my last comment I was planning on closing this PR once a proper implementation for all projects had been decided upon so the only value is in the messages we exchanged discussing the topic. |
references #3621
Description
Fix warnings
Standardize and simplify configurations
Remove unnecessary rules and settings