refactor: Use Prettier for code formatting

[ExE-Boss]

This reformats the repository using Prettier.

[Elchi3]

@schalkneethling @davidflanagan Does MDN have a style guide? Are we okay with using prettier? Do we already use it somewhere with specific settings? I don't see an ADR about it. https://github.com/mdn/mdn/tree/master/ADRs

[schalkneethling]

@schalkneethling @davidflanagan Does MDN have a style guide? Are we okay with using prettier? Do we already use it somewhere with specific settings? I don't see an ADR about it. mdn/mdn:ADRs@master

We are using Prettier with the rewrite of the frontend. We also use it for all other new SaSS, CSS, JavaSript in Kuma. We do have a config here, but I am going to throw that out and simply go with the Prettier defaults.

Does MDN have a style guide?

I am working on that as we speak. With the rewrite, it has become even more apparent to me that this is a requirement we can no longer put of. I am therefore working on my own time to define design elements, components, as well as code style.

With regards to code style specifically, we will simply use whatever Prettier does. It seems like an unnecessary pain, to try and define and then enforce our own little quirks. There are much bigger problems and challenges we can use our energy to solve ;)

[schalkneethling]

Btw. this will all live in mdn-fiori. A lot of what is there now is appropriate, but I would not invest energy in reading through and absorbing the information that is currently there. Best to hold off until I/we release something that is "official"

[Elchi3]

Thanks! In this PR the following setting are proposed: https://github.com/mdn/browser-compat-data/pull/3737/files#diff-f2e285fb35cc9694564cb6d2af099ad2

I'll wait for an MDN-wide decision and maybe an ADR before proceeding as these settings might not be the ones we'll be using throughout.

[schalkneethling]

Thanks! In this PR the following setting are proposed: mdn/browser-compat-data/pull/3737/files#diff-f2e285fb35cc9694564cb6d2af099ad2

I'll wait for an MDN-wide decision and maybe an ADR before proceeding as these settings might not be the ones we'll be using throughout.

Agreed. The JSON piece looks interesting(would need to read more about that), but for the rest, I would recommend going with the defaults. We can then disable Prettier for specific lines/blocks etc. should the default in some instances prove problematic.

[ddbeck]

@Elchi3 @schalkneethling is it possible to open an issue or a draft PR somewhere (presumably for an ADR), to track the establishment of a style standard? I don't like the idea of leaving a PR open indefinitely while waiting on a decision, but I like it even less when there's no obvious place to check whether the PR is still blocked

[schalkneethling]

@Elchi3 @schalkneethling is it possible to open an issue or a draft PR somewhere (presumably for an ADR), to track the establishment of a style standard? I don't like the idea of leaving a PR open indefinitely while waiting on a decision, but I like it even less when there's no obvious place to check whether the PR is still blocked

Yup, yup. I need to do that. Will place a link to it here.

[ExE-Boss]

I’m in favour of using prettier (preferably with tabs).

[ddbeck]

@schalkneethling is there an issue or PR to follow for a JS style decision yet? Not trying to rush the decision itself, but I want to make sure I'm keeping track of this appropriately. Thank you!

[ExE-Boss]

Well, there should probably be an ADR PR like the one for pnpm: mdn/mdn#69.

[davidflanagan]

@ExE-Boss: I think this PR is incomplete without adding prettier to devDependencies in package.json, and also adding a script to package.json so that anyone can just type npm run format to make sure everything is formatted. (Since not everyone will have their editors set up to do it automatically.)

Also, the require-pragma things makes this opt-in, and requires that @prettier comment at the top of all the files. I did it that way for my kumascript refactor. I think because I was afraid to just make it the default. In kuma now, we're doing it by default for everything in kuma/javascript/src, and it is convenient not to have to use opt-in for each file with the /** @prettier */ comment. So that is an option to consider here: whether it should be the default or something each file opts in to.

Finally, it looks like you've even got prettier running over md files. Which seems like something that might be more controversial. I don't have an opinion, really, but it might be simpler to start with just .js and .ts files.

@ddbeck: you and Florian are the ones who do the bulk of the work in this repo and I'd suggest that you should feel empowered to just make the call on this. If you think the ADR process would be useful for posterity, I'd encourage you to write up a really simple one.

But also, there are a small enough number of files affected, that you could just try it out and see how it goes. The beauty of prettier is that it is easy to change your mind about the settings and reformat things again later. The only downside is that if there is formatting churn it affects the git repo history and breaks things like git blame.

So basically, my advice is: don't over think it. But if you do decide to land this or something like it do update your package.json file with the devDependency and script.

[ExE-Boss]

@davidflanagan

I think this PR is incomplete without adding prettier to devDependencies in package.json, and also adding a script to package.json so that anyone can just type npm run format to make sure everything is formatted. (Since not everyone will have their editors set up to do it automatically.)

Way ahead of you:

browser-compat-data/package.json

Line 37 in 80e0ee1

"prettier": "^1.16.4",

browser-compat-data/package.json

Line 42 in 80e0ee1

"format": "prettier --write **/*.js **/*.ts **/*.md **/*.json",

[ddbeck]

@davidflanagan thanks for your feedback on this!

there are a small enough number of files affected, that you could just try it out and see how it goes

This is a good point. I can't speak for @Elchi3 on this but my thinking was that if I was going to wreck the git blame, it'd be nice to do it consistent with related projects. But if it's not actually on anybody's near(ish)-term agenda, then we're just avoiding having a style at all.

In the absence of a style guide, @wbamberg suggested the Airbnb ESLint style for mdn/short-descriptions#7, which has been a benign choice so far though I don't actually remember why we picked that one, apart from it being prominent. I like the idea of taking a style off the shelf and using it, though I don't know anything about prettier specifically. If @Elchi3 is OK with the prospect of having a BCD-specific style, then I'm willing to take a closer look at this particular approach.

[ExE-Boss]

Well, BCD’s style is already very close to the Prettier default, so I’d just go with that.

[ddbeck]

That's good to know! Though to be clear about myself: if the decision about a style isn't being handed down to us, then I want to be able to articulate what BCD's needs are from a style (and associated tools) and show that Prettier satisfies those needs. (It probably does—I'd imagine several styles would—but we should at least go through the motions to find out.)

[ExE-Boss]

review?(@Elchi3): I’ve rebased this.

[queengooborg]

Thanks for your enthusiasm! However, I don't think that we're quite ready to review this one just yet, based upon the previous discussion:

I'll wait for an MDN-wide decision and maybe an ADR before proceeding as these settings might not be the ones we'll be using throughout.

Don't get me wrong, I'm all in favor for enforcing a common code styling. However, we should get an MDN-wide conclusion before making such changes. (I'll be proposing an ADR for this soon, or if you want some extra points, feel free to tackle it. 😉)

[queengooborg]

Rather than propose an ADR, I opened up an issue on the main MDN repo to open further discussion and help us come to a final conclusion. (Also because I found some more info that may impact this decision.) mdn/mdn#71

[Elchi3]

Hey @ExE-Boss, it seems like the team is positive about using prettier generally. It also seems like the config doesn't matter too much but what we need most is a system in place that worries about formatting for us. That said, the config provided here should be fine and is hopefully close to how the code already formatted.
We're currently at a low amount of PRs, if you could rebase this and add the latest prettier version, I see a chance to get this in and then see how it feels. If it sticks and people are positive about it, it might also help the MDN ADR to come to a final conclusion.

[ExE-Boss]

This will probably cause #4686 to have merge conflicts, same with #4264 and #3445.

I’d prefer if we could get those two merged first and then I rebase this on top of them.

[queengooborg]

In #3445, regarding c3ba2eb, I looked around, and we actually seem to disregard the .js extension more often than not throughout BCD. This isn't something in particular to this PR, but I think we should probably considering enforcing a style on that one. (Personally, I'm in favor of omitting .js -- it feels a little more like NodeJS-style, but it's probably more of an MDN-wide decision for something like that.) @Elchi3, thoughts?

[ExE-Boss]

KumaScript has specifically made the decision to require the .js extension in mdn/kumascript#1099.

[queengooborg]

While re-reviewing this PR, a few ideas came to mind:

First, since this modifies a significant number of files, perhaps this should follow the bulk update/migration process we've recently drafted up. In other words, we should separate this PR into three: one for the changes script, one for the data updates, and one for a linter to enforce styling. In other words, perhaps this PR should only include the import and command for Prettier, as well as a script in scripts/migrations/ called 004-prettier.js (or something similar) that runs npm run format. Then, we'll have a PR for just the data alone, which will be created and merged at a scheduled time (which should eliminate any rebasing issues). Last, we'll enforce the Prettier styling in a linter file, so that we don't end up regressing.

Second, I'm also thinking that we should maybe consider throwing the format function into a fix script (totally not biassed because I wrote the fix scripts currently in the repo or anything 😛, but really, it would help reduce the use of additional commands). Since we already have a single command intended to fix all issues that a script can easily perform, we should consider attaching Prettier to it, especially if we're going to include a linter. I feel that the less commands contributors must run, the easier it will be to contribute.

Finally, someone submitted #4773 to apply formatting to all the Markdown documentation, however I decided to close it as this PR does the same thing, plus lots more (and we've already been working on this one). There is one change that the other PR introduced, however, that's not in here, which is an enforced line with of 80 characters. While I personally prefer to have no line breaks in my Markdown/rich text, I try to keep my code to 120 characters wide per line when I can. This may be something we wish to enforce in Prettier.

None of the above statements should be interpreted as me formally requesting changes to this PR as a BCD peer, but rather as suggestions from a fellow developer and contributor. 😉 @Elchi3, let me know if you agree or disagree with the above as well!

[Elchi3]

I agree @vinyldarkscratch 👍 Following the process will likely make this go through a lot smoother. I also agree about linting and thus avoiding regressing this.
Thanks for closing the other PR! These reformatting PRs are exactly the kinds of things we want to avoid by introducing a formatting lib.

[queengooborg]

Hey @ExE-Boss, do you plan to return to this PR? It would be nice to get this off the ground and commence the migration process! (I'm also happy to tackle this, while of course giving you credit, if you're unable to. 😉)

[queengooborg]

Thanks for rebasing! However, as mentioned above, we thought it would be much easier if this PR could follow the data migration process we've recently documented. It would be nice if we can get started on that, and break out the data changes from the linter/infra changes!

[ddbeck]

It looks like this has been superseded by the process started by #5031. I'm going to close this PR now, but thank you for your help getting this conversation started, @ExE-Boss! It's very much appreciated.