dprint the codebase

[jakebailey]

Be aware that elements of this stack are generated (like my modules PR); I'm going to be (intermittently) force pushing to keep it up to date as effectively any change to main causes a conflict, so apologies on that front.

---

This PR adds a dprint config to our repo. dprint (which is what deno fmt is running) is "yet another" code formatter in the same vein as prettier and others, with the main benefits of:

• Fast enough to format checker.ts, even on save, without being annoying.
• Configurable enough to format in this repo's "unique" code style (specifically, brace position after if, else, catch, etc).

This also enables us to remove effectively all eslint-based style checks, which is both a performance and ergonomics improvement. With all of the redundant style-related lint rules gone, linting the repo is now 7% faster!

The best way to view this PR is to look at the individual commits. This diff is large and makes GitHub mad, so consider viewing it github.dev or cloning it locally to play around. You'll want to install the dprint VS Code extension, which is automatically recommended per .vscode/extensions.json, and update settings.json to enable it.

---

Outstanding TODOs:

• Add CI stuff to verify that PRs are formatted.
• Figure out if we need some sort of bot action to format people's PRs.
• Figure out if we need to run dprint as part of a local test/lint run.
  • As far as I'm aware, dprint check prints a diff; a list of files or a silent mode would work a lot better here. But then again, people should just run dprint religiously, or better yet, on save. It's fast.
• Drop dprint sorting of imports in favor of simple-import-sort
• Make some final style decisions
  • Decide on a line length. This PR uses an arbitrarily high value of 1000, but many on the team have expressed their desire to lower this to something that can actually fit vertically with two columns, e.g. 120.
    • Immediate feedback from everyone is to just keep this at 1000 and we can lower it later.
  • Decide on an operator position. This PR uses "maintain" and allows them in either position. A majority of the code puts operators on the right like in:

     if (
         (someCondition && otherCondition) ||
         somethingElse()
     )

    But I personally find it much, much easier to read:

     if (
         (someCondition && otherCondition)
         || somethingElse()
     )

    • Immediate feedback is to stick with "maintain" to allow either form, and we can tweak it later.
  • Decide whether or not we are formatting JSON files.
    • This is tricky just from the PoV that dprint will remove trailing commas even in files that are JSONC. VS Code also (inconsistently) squiggles on these asking for them to be removed. But of course, like all trailing commas, they really make changing things easier.
    • This PR still has them formatted, but I may drop that for now.
  • Decide whether or not we are formatting markdown files.
    • Probably not; the format used in our markdown files is unfortunately exactly opposite to that supported by dprint/prettier. (e.g., formatters use - for bullets, but we seem to have *)
  • Decide whether or not we are formatting yaml files.
    • dprint does not support yaml files, however, it can defer to prettier and that works very well (I use this in all of my own personal projects).
• Changes in dprint that block this PR, or require us to compromise, in order of importance:
  • Single line imports with a single named bindings (import { join } from "path") are currently forced to be multiline. This is of my own doing, as the contribution I made to dprint to allow forcing multi-line imports (like we want, for good merges) was incomplete and lacks that special case.
    • feat: add config for forced multiline named imports/exports dprint/dprint-plugin-typescript#473
  • Single line functions / methods are reformatted.
    • Allow single line methods and functions dprint/dprint-plugin-typescript#3
    • We have a few of these, but not many.
  • Aligned comments in objects/enums/interface/etc become unaligned
    • Avoid de-denting comments that align dprint/dprint-plugin-typescript#410
    • It's possible that some of these cases in our repo would be better off as JSDoc, but there is precedent for this kind of behavior on the whole a la gofmt.
  • No support for ignoring a range of lines, only a specific node and children
    • Support dprint-ignore for a region dprint/dprint-plugin-typescript#411
    • Only one of these in our repo, just for aesthetics. Ignored using a block in this PR.
  • Quote props "consistently" rather than preserve or only "as needed"
    • Add prettier/eslint-like "consistent" option for quoteProps dprint/dprint-plugin-typescript#542
  • Place semicolon on multi-line union/intersections on their own line
    • On multi-line union/intersections, place the semicolon on its own line dprint/dprint-plugin-typescript#547
  • Sort imports "naturally"
    • Support sorting imports "naturally" dprint/dprint-plugin-typescript#548
    • Not a problem for us in general because we use the eslint plugin for this + grouping
• Changes in dprint I have yet to suggest upstream. I'm sure we'll think of more.
  • All reported.
• Post-merge, we need to .git-blame-ignore-revs the squash commit. I do not want to use a regular merge like last time.
• Mega optional, but run dprint on generated d.ts files.

For the opinionated choices, I have a separate PR on my fork that shows my own preference; you can check out the individual commits there: jakebailey#18

@dsherret ❤️

Fixes #18340 (in a way)

[typescript-bot]

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

[andrewbranch]

Add CI stuff to verify that PRs are formatted.
Figure out if we need some sort of bot action to format people's PRs.
Figure out if we need to run dprint as part of a local test/lint run.

I think only the first is necessary to merge the PR.

Decide on a line length. This PR uses an arbitrarily high value of 1000, but many on the team have expressed their desire to lower this to something that can actually fit vertically with two columns, e.g. 120.

Not a blocker for the PR, but 120 sounds good to me.

Decide on an operator position. This PR uses "maintain" and allows them in either position.

Not a blocker. I have no preference.

Single line imports with a single named bindings (import { join } from "path") are currently forced to be multiline.

This seems fine.

Single line functions / methods are reformatted.

I don’t feel particularly strongly about this.

Aligned comments in objects/enums/interface/etc become unaligned

Let’s ignore nodes where this is important.

[sandersn]

These are my opinions on the open questions, where different from Andrew's (they are mostly the same):

Decide on a line length. This PR uses an arbitrarily high value of 1000, but many on the team have expressed their desire to lower this to something that can actually fit vertically with two columns, e.g. 120.

I'd prefer to merge this PR with 1000 to minimise churn, and then I want to see how much churn there is for 120 vs 160 vs 200. 120 would be nice if it doesn't change things dramatically.

Decide on an operator position. This PR uses "maintain" and allows them in either position.

I prefer operators at the beginning of the line, but I think "maintain" is the right thing for getting this merged.

Single line functions / methods are reformatted.

I hope we don't have many of these! It doesn't seem to fit the style of the code as far as I know.

Formatting of json/markdown/yaml

I rarely edit json and markdown in this repo, so I'd skip those personally.
Yaml has such an evil* grammar that having a formatter sounds useful to me. But it's not urgent.

*Technically, chaotic evil. But I don't have time to make a programming language syntax alignment chart.

[jakebailey]

Tests are failing because this reformats some stuff that is reflected in baselines and stuff. Will fix later.

[sandersn]

Interesting open question: how does dprint-on-save interact with auto-save? I love autosave and would choose that over dprint-on-save if forced to choose.

[jakebailey]

Interesting open question: how does dprint-on-save interact with auto-save? I love autosave and would choose that over dprint-on-save if forced to choose.

My understanding is that vscode will only do the format if you actually hit save. Maybe there's an option to change it but I'm not so sure.

I haven't tested explicitly; I can't say I've ever accidentally had an unformatted file, though.

[jakebailey]

So on the whole, I'm fine with leaving things at 1000/maintain/etc to defer those kinds of choices until later; anything is better than the nothing we have now. I totally expected that result.

I'll drop JSON formatting out and add the prettier plugin to touch yaml;

Aligned comments in objects/enums/interface/etc become unaligned

Let’s ignore nodes where this is important.

I'll have to find these; it's unfortunately a big diff but I can probably grep and find them.

Single line functions / methods are reformatted.

I hope we don't have many of these! It doesn't seem to fit the style of the code as far as I know.

Some examples in this vein:

    return {
-       get pos() { return pos; },
-       get error() { return error; },
-       get state() { return captureMapping(/*hasSource*/ true, /*hasName*/ true); },
+       get pos() {
+           return pos;
+       },
+       get error() {
+           return error;
+       },
+       get state() {
+           return captureMapping(/*hasSource*/ true, /*hasName*/ true);
+       },

        switch (field) {
-           case "major": return new Version(this.major + 1, 0, 0);
-           case "minor": return new Version(this.major, this.minor + 1, 0);
-           case "patch": return new Version(this.major, this.minor, this.patch + 1);
-           default: return Debug.assertNever(field);
+           case "major":
+               return new Version(this.major + 1, 0, 0);
+           case "minor":
+               return new Version(this.major, this.minor + 1, 0);
+           case "patch":
+               return new Version(this.major, this.minor, this.patch + 1);
+           default:
+               return Debug.assertNever(field);
        }

Personally, I'd rather have then the "new" way anyhow. It's just a change.

[sheetalkamat]

• Drop dprint sorting of imports in favor of simple-import-sort

Not a blocker but auto import almost always results in the error. Will using dprint formatter auto fix it without having to run lint fix

• Decide on an operator position. This PR uses "maintain" and allows them in either position. A majority of the code puts operators on the right like in:

Personally I prefer the existing style

[jakebailey]

Not a blocker but auto import almost always results in the error. Will using dprint formatter auto fix it without having to run lint fix

There's currently some differences that make it so we can't use dprint to order the named bindings, unfortunately. But, if auto-import is putting things in the wrong place and the settings are set properly in settings.json, then that's a bug in TS that we should address if we can get a repro going.

[jakebailey]

Quick view of the most recent push per above: https://github.com/microsoft/TypeScript/compare/b81a0b383e031cd24e30acc5a2e5aec75f348cfc..8119de88f9645b30d54532ba8e3dfc66682bdce2

[typescript-bot]

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

[DanielRosenwasser]

But I personally find it much, much easier to read:

 if (
     (someCondition && otherCondition)
     || somethingElse()
 )

I agree but am with @sandersn on taking the incremental path.

[dsherret]

Exciting! I've been helping my friend paint after work and won't finish until Friday probably so I will take a look at everything over the weekend and try to prioritize fixing/improving things then.

[jakebailey]

Leaving a bunch of random comments.

[jakebailey]

@dsherret Is there a way to ask dprint to download the plugins in dprint.json and provide their cached locations? I'm looking into dprinting the outputs of the dts bundler, but I didn't want to add the @dprint/typescript dependency as it'd be different than what is in dprint.json, but I also don't want to have to fetch the URL from that config each time (which would then turn into a reimplementation of caching).

[dsherret]

@jakebailey the .wasm files are thrown away after they're compiled and only the compiled output is cached. It's probably easiest to call the executable (npx dprint fmt --stdin ts) and provide the text over stdin. It will respond with the formatted text on stdout and use the config found based on the cwd. Would that work? It should be quick.

[jakebailey]

I went through all of the outstanding comments, manually improving ones where it seemed to help, resolving ones that were reported or are done; are there any remaining concerns here? I assume not?

[jakebailey]

Alright, this is going in; I have disabled caching in the formatting CI task for now, pending a fix for that upstream. Locally, it won't be a problem as nobody would be copying the cache between systems.