jaredpalmer / tsdx

Zero-config CLI for TypeScript package development
https://tsdx.io
MIT License
11.24k stars 508 forks source link

Separate prettier command eg. "tsdx format" #917

Open Kamahl19 opened 3 years ago

Kamahl19 commented 3 years ago

Current Behavior

Prettier is integrated into eslint and runs in one tsdx lint command. This comes with couple of disadvantages described at https://prettier.io/docs/en/integrating-with-linters.html . It is also not possible to run prettier on non JS/TS files eg. json, css/less/scss, md, yml etc..

Desired Behavior / Suggested Solution

tsdx lint runs only lint without prettier. tsdx format runs prettier and it's possible to configure all file types we want to format. Default can be ts,tsx.. while the user can decide to also format yml, sass etc.

agilgur5 commented 3 years ago

This comes with couple of disadvantages described at https://prettier.io/docs/en/integrating-with-linters.html

TSDX does use eslint-config-prettier so the main disadvantage doesn't apply.

It is also not possible to run prettier on non JS/TS files eg. json, css/less/scss, md, yml etc..

I'm not sure if this is actually correct, I feel like there should be a way of doing this and would also like to use that feature of Prettier.

That being said, it's important to actually lint (not just format) the other types of files too, which might be outside the scope of TSDX and add bloat. I personally run a variety of linters in my IDE etc (and run Policy-as-Code and SAST in CI).

tsdx lint runs only lint without prettier.

This proposal would be a breaking change in it's current state.

To give an alternative that works today: Prettier is also very much usable in user-land too. Since it's already in your dependency tree with TSDX, you can add some package.json#scripts like:

{
   "format": "prettier --write './**/*'",
    "// format:check": "for pre-commit and CI usage",
   "format:check": "prettier --check './**/*'"
}
Kamahl19 commented 3 years ago

@agilgur5

TSDX does use eslint-config-prettier so the main disadvantage doesn't apply.

TSDX uses eslint-plugin-prettier which is the source of the disadvantages described at https://prettier.io/docs/en/integrating-with-linters.html

These plugins were especially useful when Prettier was new. By running Prettier inside your linters, you didn’t have to set up any new infrastructure and you could re-use your editor integrations for the linters. But these days you can run prettier --check . and most editors have Prettier support.

The downsides of those plugins are:

You end up with a lot of red squiggly lines in your editor, which gets annoying. Prettier is supposed to make you forget about formatting – and not be in your face about it!
They are slower than running Prettier directly.
They’re yet one layer of indirection where things may break.

To give an alternative that works today

Yes this is of course possible but then I have 2 ways to run prettier and things get confusing for less experiences people. Hiding prettier behind lint is not transparent and requires these workarounds.

I understand it would be a breaking change.

agilgur5 commented 3 years ago

TSDX uses eslint-plugin-prettier which is the source of the disadvantages described at https://prettier.io/docs/en/integrating-with-linters.html

Yes, I've read that section, but aside from the performance piece, that section is all an opinion on usability. Some people definitely prefer using eslint-plugin-prettier and having a single tool, ESLint, which already does formatting. So it has trade-offs and calling that a disadvantage would be a matter of opinion.

things get confusing for less experiences people

IMO having 2 different tools doing similar things (and tsc for type-checking) is already complex enough to confuse less experienced engineers. Either way, you'd need multiple scripts, TSDX is just making a quite small abstraction over them.

Hiding prettier behind lint is not transparent and requires these workarounds.

Well TSDX's purpose is to hide a lot of things behind an abstraction. I would say "workaround" is a bit of a strong term given that eslint-config-prettier is still needed regardless and eslint-plugin-prettier has 4.5M weekly downloads, a little less than half of prettier's 11.5M. It's not really a workaround if it makes up half the userbase 😅

I'm open to changing this if there's enough support for it, but I don't think that'll happen on a near-term timeline.

Kamahl19 commented 3 years ago

Disclaimer: I understand tsdx IS opinionated tool and while this request is (imho) based on some facts, it's still mostly about my opinion.

One of the points of the prettier is that user doesnt need to care about formatting at all, so showing bunch of red errors in editor while writing the code is just bad DX. I would say most of the devs just write js/ts code and hit save to auto-format the code. So when they see red errors coming from lint, they assume they are breaking some lint rules while its just formatting.

It also adds more dependencies to tsdx which need to be updated and another layer of abstraction which might break when upgrading to major versions of eslint / prettier. When you have "clean & separate" eslint and prettier, it's easier to upgrade and to think about the tools.

I personally believe in single-responsibility principle which means lint does linting and format does formatting. How can single responsibility make things confusing :) Usually its the other way round.

agilgur5 commented 3 years ago

It also adds more dependencies to tsdx

eslint-plugin-prettier is only a single dependency out of the gigantic amount of other deps. It can be impacted by majors, yes, but so will eslint-config-prettier, which is necessary. So not a huge burden in that sense.

I personally believe in single-responsibility principle which means lint does linting and format does formatting.

Well eslint --fix and ESLint style rules existed well before Prettier or really "formatter tools" as a category existed -- so it already does both and will continue to do both. I've personally been a long-time user of Standard, which is entirely built on ESLint. Also there's a group of people that believe type-checking is a subset of linting too in #352 -- there's opinions on opposite ends of the spectrum here.

But as I wrote there combining tools typically means for more bugs, nuances, and hacks and is well-evidenced in TSDX issues itself (some mentioned there too), so I'm much more inclined to separate them and am in fact planning to separate each command as a separate package and configs as separate presets (#634 ).

ackvf commented 2 years ago

@Kamahl19 I wonder, how do you see the red errors coming from prettier? It doesn't work for me. I just installed new tsdx project, changed printWidth in package.json to 10 to see some errors and nothing. This is what I get.

image

Only after I run npm run lint I get the errors

C:\Users\Qwerty\repos\BigN>npr lint

> bign@0.1.0 lint
> tsdx lint

Defaulting to "tsdx lint src test"
You can override this in the package.json scripts, like "lint": "tsdx lint src otherDir"

C:\Users\Qwerty\repos\BigN\src\index.ts
  1:21  error  Replace `a:·number,·b:·number` with `⏎··a:·number,⏎··b:·number⏎`                                                          prettier/prettier
  2:7   error  Replace `'development'·===·process.env.NODE_ENV` with `⏎····'development'·===⏎····process⏎······.env⏎······.NODE_ENV⏎··`  prettier/prettier
  3:17  error  Replace `'boo'` with `⏎······'boo'⏎····`                                                                                  prettier/prettier
  5:10  error  Replace `a·+·b` with `(⏎····a·+·b⏎··)`                                                                                    prettier/prettier

C:\Users\Qwerty\repos\BigN\test\blah.test.ts
  5:12  error  Replace `sum(1,·1)).toEqual(2` with `⏎······sum(⏎········1,⏎········1⏎······)⏎····).toEqual(⏎······2⏎····`  prettier/prettier

✖ 5 problems (5 errors, 0 warnings)
  5 errors and 0 warnings potentially fixable with the `--fix` option.

Autofix on commit doesn't work either. I had to add new command to package.json scripts.

    "lint:fix": "tsdx lint --fix",