这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@albertothedev
Copy link
Contributor

references #3621

Description

Fix warnings
Standardize and simplify configurations
Remove unnecessary rules and settings

@albertothedev albertothedev requested a review from a team as a code owner March 12, 2023 17:18
@turbo-orchestrator turbo-orchestrator bot added area: docs Improvements or additions to documentation area: examples Improvements or additions to examples pkg: create-turbo Issues related to npx create-turbo team: turborepo labels Mar 12, 2023
@vercel
Copy link
Contributor

vercel bot commented Mar 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
examples-basic-web ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 12, 2023 at 5:19PM (UTC)
examples-cra-web ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 12, 2023 at 5:19PM (UTC)
examples-designsystem-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 12, 2023 at 5:19PM (UTC)
examples-kitchensink-blog ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 12, 2023 at 5:19PM (UTC)
examples-nonmonorepo ❌ Failed (Inspect) Mar 12, 2023 at 5:19PM (UTC)
examples-svelte-web ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 12, 2023 at 5:19PM (UTC)
examples-tailwind-web ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 12, 2023 at 5:19PM (UTC)
examples-vite-web ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 12, 2023 at 5:19PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
examples-native-web ⬜️ Ignored (Inspect) Mar 12, 2023 at 5:19PM (UTC)

@vercel
Copy link
Contributor

vercel bot commented Mar 12, 2023

@albertothedev is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@mehulkar mehulkar left a 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?

@albertothedev
Copy link
Contributor Author

albertothedev commented Mar 16, 2023

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 non-monorepo example, in case you haven't noticed. It's the only one to do so and I don't understand why. All its scripts (lint, dev, start, and build) are working on my machine.

Copy link
Member

@tknickman tknickman left a 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.

  1. 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 turbo automatically understand when your cache should be busted. By default, turbo automatically 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.
  2. 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:
      1. Starting with the basic example
      2. Add a console statement to an app
      3. Run turbo lint (passes, because our lint config allows console statements)
      4. Add "no-console": "error", to the root config
      5. 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.

  3. 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!

@albertothedev
Copy link
Contributor Author

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.

  1. 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 turbo automatically understand when your cache should be busted. By default, turbo automatically 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.
  2. 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:

      1. Starting with the basic example
      2. Add a console statement to an app
      3. Run turbo lint (passes, because our lint config allows console statements)
      4. Add "no-console": "error", to the root config
      5. 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.

  3. 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!

Introduction

First 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:

... 3 eslint configs in our examples (a shared config in packages/, a root/ config, and a config in the app itself)...

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:

Glossary

When a word refers to a term specified here, it will be bold.

  • configuration: refers to a package where an ESLint configuration (rules, parsers, overrides, etc.) is defined for its use elsewhere in the monorepo.
  • root extender: refers to the ESLint file at the root of the monorepo that extends a configuration.
  • project extender: refers to an ESLint file inside an app or package that extends a configuration.

Example setups

For 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 with-nextjs, which is just a readme pointing to the examples that use Next.js. This table describes how each one of them is set up using ESLint.

# Name Configurations Root extender? Project extenders
1 basic 1 ✔️ 2 (apps/docs, apps/web)
2 design-system 1 ✔️ 3 (apps/docs, packages/acme-core, packages/acme-utils)
3 kitchen-sink 2 (custom, custom-server) 5 (apps/admin (custom), apps/api (custom-server), apps/storefront (custom), packages/logger (custom), packages/ui (custom))
4 non-monorepo None ✔️ None
5 with-changesets 1 ✔️ 3 (apps/docs, packages/acme-core, packages/acme-utils)
6 with-create-react-app 1 1 (packages/ui)
7 with-docker 2 (custom, custom-server) ✔️ (custom) 3 (apps/api (custom-server), apps/web (custom), packages/logger (custom))
8 with-npm 1 ✔️ 2 (apps/docs, apps/web)
9 with-prisma 1 ✔️ 1 (packages/database)
10 with-react-native-web None None
11 with-rollup 1 ✔️ 1 (apps/web)
12 with-svelte 1 ✔️ 3 (apps/docs, apps/web, packages/ui)
13 with-tailwind 1 ✔️ 2 (apps/docs, apps/web)
14 with-vite 1 ✔️ 3 (apps/docs, apps/web, packages/ui)
15 with-yarn 1 ✔️ 2 (apps/docs, apps/web)

Questions and concerns

  1. The with-react-native-web example isn't linted. It doesn't even have a lint command. It is very similar to the non-monorepo, which just extends Next.js's ESLint plugin, so maybe the same could be done here using eslint-plugin-react-native? I haven't worked with React Native so I don't know what the correct choice is here.

  2. Your second point, explaining that the root extender shouldn't be used for configuration options because it is not tracked by Turborepo, therefore causing the cache to miss any changes to it, is extremely important and, as far as I can tell, isn't documented anywhere. This isn't a huge problem because there are literally no upsides to configuring ESLint there when doing so on the configuration will act the same way, however, since the caching limitation with this setup isn't documented, a user could make that mistake. A simple fix would be to add a comment on the root extender explaining the situation, like so:

    // Do NOT configure ESLint (rules, parsers, overrides, etc.) here as Turborepo is not aware of this file's existence, meaning that it won't detect changes to it and the cache will skip it. Use the shared config in `packages` as Turborepo knows about it and extend it here.
    module.exports = {
      root: true,
      // This tells ESLint to load the config from the package `eslint-config-custom`
      extends: ["custom"],
    };

    But explaining it on the documentation as well would be even better.

  3. You say on your second point that the root extender is there for editor support but I am testing it and I don't think it's necessary. At least not on VSCode:

    1. Open examples/basic on VSCode. As it is, there's a root extender, three linted files: packages/ui, apps/docs, and apps/web, with only the latter two having project extenders. We will be testing by adding the no-console to either the root extender or the configuration and adding a log statement to both apps/docs and packages/ui, to see the effect of root extenders and project extenders on editor support. For starters, let's add the log statements and nothing else and check that there are neither IDE nor command ESLint errors:

      Screenshot_20230330_190316

    2. Add the rule to the root extender. We now can see both an IDE and command ESLint errors on packages/ui but none on apps/docs. This expected behavior. The root extender applies the conflicting rule to the entire monorepo but the project extender in apps/docs overwrites it with the configuration's setup, which doesn't have it.

      Screenshot_20230330_190221

    3. Remove the rule from the root extender and add it to the configuration. As expected, we see no errors in packages/ui since it doesn't have a project extender. Also as expected, we see an ESLint error when running the lint command in apps/docs, but we also see VSCode is aware of it.

      Screenshot_20230330_191840

    This means there's no need to use root extenders for editor support and project extenders will work. So now the question is: how should we set up the examples? Let's look at the structure of basic:

    basic
    ├── apps
    │   ├── docs
    │   │   └── .eslintrc.js
    │   └── web
    │       └── .eslintrc.js
    ├── packages
    │   ├── ui
    │   └── eslint-config-custom
    └── .eslintrc.js
    

    My original question asked why there are project extenders on apps/docs and apps/web when with a root extender none were needed. From what we discussed then, I decided that removing them to avoid confusion was the best idea, so I have made these changed on this PR:

    basic
    ├── apps
    │   ├── docs
    │   └── web
    ├── packages
    │   ├── ui
    │   └── eslint-config-custom
    └── .eslintrc.js
    

    This is great, but it won't work well with monorepos that use more than one ESLint configuration. We have two such examples: kitchen-sink and with-docker. We'll look at kitchen-sink since it's the most complex one. This is its current state:

    kitchen-sink
    ├── apps
    │   ├── admin
    │   │   └── .eslintrc.js (custom)
    │   ├── api
    │   │   └── .eslintrc.js (custom-server)
    │   └── storefront
    │       └── .eslintrc.js (custom)
    └── packages
        ├── logger
        │     └── .eslintrc.js (custom)
        ├── ui
        │   └── .eslintrc.js (custom)
        ├── eslint-config-custom
        └── eslint-config-custom-server
    

    Best we can do if we want to do it the root extender way is pick one of the configurations to extend on the root extender and apply to everything and then use project extenders for the apps and `packages** that require others. It's not possible to always use only root extenders, but it is possible to always only use project extenders.

    So we have three options for structuring the examples:

    1. Root extenders as a priority. When an example can't only use a root extender, pick one configuration to extend on the root extender and use project extenders for the apps and packages that require others. basic and kitchen-sink examples:

      basic
      ├── apps
      │   ├── docs
      │   └── web
      ├── packages
      │   ├── ui
      │   └── eslint-config-custom
      └── .eslintrc.js
      
      kitchen-sink
      ├── apps
      │   ├── admin
      │   ├── api
      │   │   └── .eslintrc.js (custom-server)
      │   └── storefront
      ├── packages
      │   ├── logger
      │   ├── ui
      │   ├── eslint-config-custom
      │   └── eslint-config-custom-server
      └── .eslintrc.js (custom)
      
    2. Root extenders only or project extenders only. When an example only needs a root extender, use that, but when it requires one or more project extenders, i.e., has more than one configuration, use only those. basic and kitchen-sink examples:

      basic
      ├── apps
      │   ├── docs
      │   └── web
      ├── packages
      │   ├── ui
      │   └── eslint-config-custom
      └── .eslintrc.js
      
      kitchen-sink
      ├── apps
      │   ├── admin
      │   │   └── .eslintrc.js (custom)
      │   ├── api
      │   │   └── .eslintrc.js (custom-server)
      │   └── storefront
      │       └── .eslintrc.js (custom)
      └── packages
          ├── logger
          │   └── .eslintrc.js (custom)
          ├── ui
          │   └── .eslintrc.js (custom)
          ├── eslint-config-custom
          └── eslint-config-custom-server
      
    3. Project extenders only. Only use project extenders, regardless of the example setup. basic and kitchen-sink examples:

      basic
      ├── apps
      │   ├── docs
      │   │   └── .eslintrc.js
      │   └── web
      │       └── .eslintrc.js
      └── packages
          ├── ui
          │   └── .eslintrc.js
          └── eslint-config-custom
      
      kitchen-sink
      ├── apps
      │   ├── admin
      │   │   └── .eslintrc.js (custom)
      │   ├── api
      │   │   └── .eslintrc.js (custom-server)
      │   └── storefront
      │       └── .eslintrc.js (custom)
      └── packages
          ├── logger
          │   └── .eslintrc.js (custom)
          ├── ui
          │   └── .eslintrc.js (custom)
          ├── eslint-config-custom
          └── eslint-config-custom-server
      

    The third option, "project extenders only", is the only one that allows us to ensure consistency across any example, which means the user is presented with the simplest pattern for linting their apps and packages. It is also the least error-prone, since by ditching the use of root extenders it also removes the need for the user to be aware of their caching limitations. Finally, it is the easiest to document.

Let me know what you think.

@albertothedev
Copy link
Contributor Author

@tknickman @mehulkar

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?

@tknickman
Copy link
Member

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!

@tknickman tknickman closed this Sep 7, 2023
@albertothedev
Copy link
Contributor Author

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.

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

Labels

area: docs Improvements or additions to documentation area: examples Improvements or additions to examples pkg: create-turbo Issues related to npx create-turbo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants