ckolderup / postmarks

a single-user bookmarking website designed to live on the Fediverse
https://postmarks.glitch.me
MIT License
459 stars 38 forks source link

Apply prettier fixes #94

Closed ckolderup closed 10 months ago

ckolderup commented 10 months ago

This is a followup to @PatOConnor43's PR #83 which added eslint to the codebase; I decided to also add Prettier in hopes that we can avoid formatting noise in future changes.

Reviewing the actual diff is probably not super useful beyond the first commit (e8f7661), but I do have a couple questions/thoughts that I'd be interested in hearing opinions on from anyone who's contributed/plans on contributing/is experienced with collaborating on modern JS projects.

1) One thing that's not clear to me about putting a .vscode/settings.json into a project is that if I make a setting that refers to a particular extension, will it prompt the user to install that extension? I'd love for anyone else who's using VSCode to benefit from everything "just working" but I don't know if I'm going to make things more broken somehow.

2) There are a few things that aren't the most recent version, because the balance of making Prettier, eslint, and the most popular Prettier VSCode extension all work together required some specific versions, annoyingly. Is there a better combination of these that I should be using instead? I tried a different VSCode extension that was specifically marketed as being for eslint + prettier, but ran into even more problems trying to get that working on my setup.

Update: thanks to @pburke's detailed comments here, this now also closes #70.

pburke commented 10 months ago
  1. With this config, VSCode will not prompt the user to install the Prettier extension. To do that you can click the gear icon by the extension name, then select "Add to Workspace Recommendations" OR manually create .vscode/extensions.json with:

    {
    "recommendations": ["esbenp.prettier-vscode"]
    }
  2. This setup works for me, using VSCode + Prettier extension. Strictly speaking, eslint-plugin-prettier is only necessary if you want to support another workflow where eslint (rather than the user's IDE) is responsible for invoking Prettier. If you are only interested in making sure eslint and Prettier don't fight with each other, all you need is eslint-config-prettier, which just disables any eslint rules that would cause problems (see: Prettier vs. Linters).

Either way, I think the eslint config can be simplified a bit:

With eslint-plugin-prettier (so folks can optionally run prettier through eslint) Move `plugin:prettier/recommended` to the **end** of the extends list. This recommended config will automatically include the relevant plugins and rules settings, including everything that `eslint-config-prettier` does. ```JSON { "env": { "browser": true, "es2021": true }, "extends": ["airbnb-base", "plugin:node/recommended", "plugin:prettier/recommended"], "parserOptions": { "ecmaVersion": "latest", "sourceType": "module" }, "rules": { "no-unused-vars": "warn", "no-console": "off", "func-names": "off" } } ```
With eslint-config-prettier (so eslint plays nice with, but does not invoke, prettier) Just have `prettier` as the **last thing** in extends. You don't need prettier in plugins or rules. If you go this route, you can also `npm remove eslint-plugin-prettier --save-dev` ```JSON { "env": { "browser": true, "es2021": true }, "extends": ["airbnb-base", "prettier"], "parserOptions": { "ecmaVersion": "latest", "sourceType": "module" }, "rules": { "no-unused-vars": "warn", "no-console": "off", "func-names": "off" } } ```

Other stuff:

ckolderup commented 10 months ago
  1. With this config, VSCode will not prompt the user to install the Prettier extension. To do that you can click the gear icon by the extension name, then select "Add to Workspace Recommendations" OR manually create .vscode/extensions.json with:
{
  "recommendations": ["esbenp.prettier-vscode"]
}

👍 thanks! adding.

  1. This setup works for me, using VSCode + Prettier extension. Strictly speaking, eslint-plugin-prettier is only necessary if you want to support another workflow where eslint (rather than the user's IDE) is responsible for invoking Prettier. If you are only interested in making sure eslint and Prettier don't fight with each other, all you need is eslint-config-prettier, which just disables any eslint rules that would cause problems (see: Prettier vs. Linters).

I think I'm down to keep it so that a single npm run lint will also incorporate prettier.

  • Looks like eslint-plugin-import is not used and can be removed from package.json?

good catch, I was messing with it at some point while I was getting an error about one of the imports in the package and didn't clean up properly.

  • Re: versions, I was able to npm install eslint-config-prettier@9 --save-dev to get the latest version. No problems so far.

fair enough, I'll double-check in my environments. Maybe something else is messy with my VSCode settings, I've noticed that years of settings sync has done some strange things to my settings.

  • It probably makes sense to start using Node 18+. Node 16 just went EOL on Monday. Node 20 will be the new LTS version starting in October.

I documented this in #70, but unfortunately Glitch isn't offering >16 yet despite the EOL-- big upgrade effort for a number of things are in the works there, but uplifting that entire ecosystem is a big task. I did manage to look up how to specify version ranges, and I'll go ahead and include that update to the package.json in this PR for now.

Thanks for all the detailed help on this one.

ckolderup commented 10 months ago

Given the sheer quantity of changes here and the lack of tests, I'm going to put this branch onto a fresh Glitch install ASAP and try to walk through a full round of manual QA (and make a list of what I'm doing while I'm doing it). Hopefully this will help confirm that everything still works (I just spun it up locally but currently rely on Glitch for QAing the ActivityPub stuff) and also help hammer into my brain the urgency with which writing tests would help with these kinds of things.

ckolderup commented 10 months ago

Tested on a Glitch install and seems to be working smoothly. Wrote out a manual QA punchlist which could also serve as an eventual scaffold for testing and documentation?

ckolderup commented 10 months ago

Gonna go ahead and merge on benevolent dictator privileges to unblock some other activity; my question above still stands, but testing all seems fine.