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

Add dprint code formatter #7263

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Add dprint code formatter #7263

wants to merge 2 commits into from

Conversation

wgoehrig
Copy link
Member

@wgoehrig wgoehrig commented Oct 14, 2024

The primary reason for choosing dprint was performance - check out the video at https://dprint.dev/overview/ to see just how fast format on save can be.

This PR adds a new rush fmt command that will run dprint fmt against the entire monorepo, but that's probably the slowest way to run dprint, since rush and node add a lot of unnecessary startup overhead. In fact, on my machine running rush fmt is at least 10x slower than just running ./tools/build/node_modules/dprint/dprint fmt from the root of the repo. In fact, dprint fmt was able format every file in the repo in under a second!

Either way, I recommend just setting up your editor of choice to run dprint fmt on save and never worry about formatting again. I've added dprint as a recommended VSCode extension and setup format-on-save in the VSCode workspace settings, although you'll want to make sure editor.defaultFormatter is not set differently in your user settings as that will override the workspace.

As for the actual formatting, I've tried to configure dprint to match our existing formatting as much as possible - we can always choose more opinionated settings if folks are OK with more initial churn. Let's get all the arguing about style rules out of our system now and not look back.

Still to do/open questions:

  • Run dprint check in CI
  • Remove stylistic lint rules
  • Do we want a dprint check pre-commit hook?
  • Add a .git-blame-ignore-revs file to hide the formatting from git blame.
    • Need to do that in a separate PR since this will be squashed.
    • Shouldn't we split the install/configure and actual formatting into two PRs if we're going to hide this from blame?

@pmconne
Copy link
Member

pmconne commented Oct 14, 2024

Have you checked yet to see if the dprint formatted code conflicts with any of our formatting-related lint rules?
Ideally we could remove all such rules and let the formatter worry about formatting and the linter worry about code.

@wgoehrig
Copy link
Member Author

wgoehrig commented Oct 14, 2024

Ideally we could remove all such rules and let the formatter worry about formatting and the linter worry about code.

Many of those rules were removed in the latest version of typescript-eslint (stylistic eslint rules overall are now discouraged by typescript-eslint), so I do plan on removing all stylistic lint rules, which should hopefully also speed up lint times.

@wgoehrig
Copy link
Member Author

This is blocked by swc-project/swc#9688

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.

2 participants