challenge(formatter): add bracketSpacing option matching Prettier's setting

[faultyserver]

Summary

This PR adds a new bracketSpacing option for the js formatter which matches Prettier's equivalent option. This option controls whether spaces are inserted around "object-like literals", meaning object expressions, named import/export specifier lists, import assertions, and type expressions.

The following is a case that should cover pretty much every variation that this option affects:

import {a, b, c} from "foo" assert {type: "json"};

type X = {a: string};
type Y = {
  a: string;
};

const {a, b} = {a, b};
let {foo, bar, baz, zzz, andareallylongname} = {
  foo,
  bar,
  baz,
  zzz,
  andareallylongname,
};

export {a, b as car};

type Foo<A extends {type: string}> = A<{type: B}>;

With bracketSpacing: false, this code should appear unchanged after formatting.

By default, bracketSpacing is left as true, just like in Prettier, and users will have to explicitly disable it to see any changes from their current formatting.

The goal of adding this option is to ease adoption from existing Prettier users. While Biome does not (and I'm assuming won't) aim to have perfect parity with Prettier's features, bracket spacing is a commonly-set option, and one that causes a massive diff for large codebases.

Not much other discussion has happened on this option, other than this post from a little over a month ago: #392.

Implementation Notes

I'm both relatively new to Rust and completely new to this project, so I looked for inspiration on how to add an option by finding a previous addition, arrowParentheses, and copying over the plumbing, then implementing the feature, and finally adding the relevant tests.

I decided to use a plain bool value for the option rather than a string/enum only because that's what Prettier does. If a string value would be preferred, I'm happy to change it to match those as well.

Test Plan

cargo test with a bunch of new snapshots. A local playground link with the sample code above also demonstrates the behavior.

[faultyserver]

I added this conditional_space function just for clarity at the callsites. Not sure if it has too much utility outside of this option, but I like the "branchless" flow from the caller side while still making it clear that the space may or may not exist.

[Conaclos]

Instead of using conditional_space(cond) at call site, did you try cond.then_some(space())?

[ematipico]

Instead of using conditional_space(cond) at call site, did you try cond.then_some(space())?

That wouldn't be very ergonomic when writing the IR, and it saves us a bunch of branching.

I can totally see the usage of this builder because our infra can understand Option. I would call this function maybe_space (sorry for the bike-shedding) because it's a prefix we already used through the code base,.

[faultyserver]

I can definitely agree with maybe_*. Honestly idk why I didn't go with that in the first place since it's normally what I end up using lol.

I didn't actually know .then_some existed when I wrote this code (still very new to Rust), and had I known I might've used that instead. But I do like the declarative nature of saying maybe_ for things, along with the advantage of working with any boolean value, not just an Option.

PR should be updated with the name change now.

[Conaclos]

@faultyserver you can be interested by #720

[faultyserver]

Happy to turn this into a challenge feature! I hadn't seen anything about whether there was a goal to support all of Prettier's options as part of the challenge or not.

[SuperchupuDev]

@faultyserver FYI we just got confirmation that the challenge does aim for all prettier js options, after we asked for clarification

[faultyserver]

I ended up basically re-implementing the feature after trying to rebase off main and somehow ended up with 20 fewer files changed lol.

But I think this is now ready to go again pending CI.

[ematipico]

The implementation looks good, and fortunately, this option doesn't add much friction in terms of maintenance.

I added some comments we should address before merging, although I am very pleased to see this contribution, thank you @faultyserver !

[ematipico]

For arguments that are true/false, at the moment we use different implementation, e.g.

    #[bpaf(long("vcs-use-ignore-file"), argument("true|false"), optional)]
    pub use_ignore_file: Option<bool>,

We could change all of them to use BOOLEAN, but for now we should be more consistent. .

[ematipico]

Instead of using conditional_space(cond) at call site, did you try cond.then_some(space())?

That wouldn't be very ergonomic when writing the IR, and it saves us a bunch of branching.

I can totally see the usage of this builder because our infra can understand Option. I would call this function maybe_space (sorry for the bike-shedding) because it's a prefix we already used through the code base,.

[ematipico]

Same as the previous suggestion: soft_block_indent_with_maybe_space

[SuperchupuDev]

you forgot one minor detail, but other than that, looks good!

[ematipico]

Great! We just need to fix the conflicts with the new options we just added and we can merge the PR! Again, great work here

[SuperchupuDev]

i can't really properly review a PR like this without my browser tab lagging but i'm approving to remove the previous requested changes review. great work! thanks a lot for contributing ❤

[faultyserver]

should hopefully be updated with no conflitcs now 🤞

[faultyserver]

i should stop assuming before CI passes, but now finally it's all set.