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

Feature: add default prettier config #25

Closed
wants to merge 2 commits into from

Conversation

psimk
Copy link

@psimk psimk commented Jan 14, 2022

No description provided.

@ThaNarie
Copy link
Member

Does adding this config to eslint specifically mean that we don't have a prettier config file in the project anymore?

If not correct, there is duplication.

If correct, what about other files that eslint doesn't support, but prettier does?

  • CSS, Less, and SCSS (I guess this can be configured in stylelint-config-prettier)
  • HTML
  • JSON (I believe eslint supports json as well?)
  • GraphQL
  • Markdown, including GFM and MDX
  • YAML

For formatting those, we do want prettier as a separate config file – including editor integration.

Also, if you read https://prettier.io/docs/en/integrating-with-linters.html, they discourage enabling prettier rules in eslint, and instead just run prettier to fix things. Although I'm not completely sold on that (it would require running both prettier and eslint separately, since eslint fixes more things than prettier... not sure how that nicely integrates with editors as well).

Would a better option maybe be to publish a prettier config package, and include that in skeletons as an "extends"?

{
files: "*.json",
options: {
printWidth: 999999,

Choose a reason for hiding this comment

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

Does this value means json files will be written in a single line?

Copy link
Author

Choose a reason for hiding this comment

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

this means that they can be written in a single line, which now thinking about it does not make a lot of sense.

Copy link
Member

Choose a reason for hiding this comment

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

It might have been a leftover from an older version of prettier where long values where wrapped to the next line.

so:

{
  "key": "value .............. very long ..............."
}

would become

{
  "key":
    "value .............. very long ..............."
}

Just tested in the REPL, it doesn't do that anymore. So I think we can remove it.

OR, it was copied over from one of the projects where we generated mock files that were API responses (without formatting), and we didn't want prettier to constantly handle those. Either way, we can remove it.

@psimk
Copy link
Author

psimk commented Jan 17, 2022

Does adding this config to eslint specifically mean that we don't have a prettier config file in the project anymore?

Yes. This would be the default prettier configuration used by eslint, and it could still be overwritten by a .prettierrc file.

For formatting those, we do want prettier as a separate config file – including editor integration.

I did overlook this, running prettier standalone would not take the configuration from this eslint config.

Also, if you read https://prettier.io/docs/en/integrating-with-linters.html, they discourage enabling prettier rules in eslint, and instead just run prettier to fix things.

We have a mix of this in our skeletons: eslint does prettier formatting through eslint-plugin-prettier and we also have a format script that runs prettier standalone. I think that generally, developers run both eslint linting and prettier formatting, but if our current eslint config handles prettier formatting, the latter does not make sense.

Perhaps it would be indeed better to ship a separate prettier configuration and simply disable eslint formatting rules, so eslint would be responsible for linting and prettier for formatting. I think that modern editors should be able to handle this, at least vscode and neovim both support running multiple "on save" or "on type" actions.

@ThaNarie
Copy link
Member

I think that generally, developers run both eslint linting and prettier formatting, but if our current eslint config handles prettier formatting, the latter does not make sense.

Personally (using Webstorm IDE) I only run "eslint" on safe (which formats those files that are supported by eslint).
For other files, it would probably be useful to run prettier on save as well, but that's duplicated work for "eslint supported files". Although a you can configure the extensions to run either tool on.

running multiple "on save" or "on type" actions
Downside if running both eslint and prettier on the same file, is that the AST parsing needs to happen twice :(

Either way, I still think a .prettierrc file should exist in all projects. The question is, does that mean we should leave it out in the eslint config, or should it be included for in case it's used without a prettier config, and should still work correctly in isolation?

@psimk
Copy link
Author

psimk commented Jan 17, 2022

Downside if running both eslint and prettier on the same file, is that the AST parsing needs to happen twice :(

I don't think that would impact things too much, on the same page https://prettier.io/docs/en/integrating-with-linters.html, the maintainers mention:

The downside is that these tools are much slower than just running Prettier.

I ran some benchmarks on one of our larger projects which is using the react config and these are the results I got:

base config: 2m 36s
image

config with "prettier/prettier": "off": 2m 1s
image (1)

prettier standalone: 17s
image (2)

So it seems that, running eslint without prettier configured and then running prettier separately is faster, than running prettier under eslint. Note that both eslint runs were ran without any sort of cache and both were configured to run only on .ts and .tsx files.

Either way, I still think a .prettierrc file should exist in all projects.

I agree with this, but given the performance implications I would seriously consider dropping running prettier under eslint and running it standalone. Especially, as you mentioned previously, we would likely still want prettier to format files which are not supported by eslint.

@ThaNarie
Copy link
Member

So it seems that, running eslint without prettier configured and then running prettier separately is faster, than running prettier under eslint.

Looking at timings from other rules, there is some difference between the two tests (so nothing conclusive). But I agree the difference is neglectable for this discussion – running them separately results in similar enough speeds.

And I think I agree that we should configure and run them separately.

So that would mean;

  • remove prettier from eslint (but keep the preset that disables all the rules that prettier fixes)
  • publish a prettier config
  • document the usage of prettier and eslint – and how to configure them in the IDE

@psimk
Copy link
Author

psimk commented Jan 18, 2022

closed in favor of #26

@psimk psimk closed this Jan 18, 2022
@psimk psimk deleted the feature/add-default-prettier-config branch March 14, 2022 12:04
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.

None yet

3 participants