anthonyshew / maestros

https://shew.dev
127 stars 8 forks source link

Check with Josh Goldberg about my ts-eslint doc. #53

Closed anthonyshew closed 9 months ago

anthonyshew commented 10 months ago

He's the best so mind as well ask. :)

JoshuaKGoldberg commented 9 months ago

Ok dokie! Thanks for pinging me on this 😊 it's nice to be called out positively. Plus the opportunity to suggest typescript-eslint usage is useful too!

Assorted thoughts in order of document appearance...

ts-eslint

We normally try to say “typescript-eslint” in full, but also that’s a mouthful. Was calling it ts-eslint for short an intentional choice? If so, I’d suggest explicitly saying something like “or ts-eslint for short” before using the term on its own for the first time.

Though, come to think of it, I don't think any other learning resources refer to typescript-eslint as ts-eslint. I'd recommend saying the name in full.

typescript-eslint is an important addition to a monorepo, bringing more power to ESLint through the type checking of TypeScript

So there are really four relevant things in typescript-eslint, in order from "necessary" to "useful":

  1. You literally can’t use ESLint on code with TypeScript syntax without @typescript-eslint/eslint-parser
  2. Many core ESLint ESLint rules need to be swapped out with @typescript-eslint/eslint-plugin's extension rules
  3. The plugin additionally comes with rules that give additional benefit for TypeScript syntax
  4. The plugin also comes with rules that give additional benefit when the parser can be told how to generate type information (parserOptions.project)

The phrasing in the maestros page now seems to imply only 4. I'd recommend being explicit about 1. IME users have a tendency to punt perceived nice-to-haves (3-4) and then forget about them, even when tooling is broken (1-2).

bringing more clarity to your codebase

What do you mean by this?

an excellent breakdown on how to use ts-eslint in a monorepo

Aww thanks! ❤️

Nit: probably want to remove the /#one-tsconfigjson-per-package-and-an-optional-one-in-the-root from the URL?

... now be able available in our ...

Typo: remove able?

project: ./tsconfig.json,

How about project: true ✨? A very timely blog post is releasing tomorrow: https://main--typescript-eslint.netlify.app/blog/parser-options-project-true. true has been available for a few months now though.

extends: [require.resolve('@repo/lint/next.js')]

Yup this is a great solution for linting in a monorepo. Really clean, love it - and the pedagogy up to this point has been great.

Just like that, you may have some new linting errors to clean up!

Not quite. The guide is missing enabling any new lint rules. Without enabling rules and/or extending from a preset config, users won't see any new complaints.

I think you'll want to explicitly show a line like:

extends: ['eslint:recommended', 'plugin:@typescript-eslint/recommended-type-checked'],

Even if users do read the linked typescript-docs, I'd expect many of them would be confused about how to use extends if it's not explicitly shown in the maestros docs.


Overall thoughts:

  1. Very clean and understandable narrative flow in general, nice! 👏
  2. It seems to conflate the "must use" parts of typescript-eslint with the "nice to have" new rules?
  3. Missing the extends means users won't see any benefits yet out-of-the-box
anthonyshew commented 9 months ago

Wow, this is all so tasty. There's like four things I learned today in here.

Thank you again for taking the time! typescript-eslint to the moon! 😄