swup / fragment-plugin

A swup plugin for dynamically replacing containers based on rules 🧩
https://swup-fragment-plugin.netlify.app
MIT License
15 stars 1 forks source link

feat: auto-format using lint-staged and husky #71

Closed hirasso closed 5 months ago

hirasso commented 5 months ago

Description

Auto-format source and markdown files before committing changes to them. Uses lint-staged together with a husky pre-commit hook so that the formatting only runs on staged files before committing them, ignoring all other files (→ speed!!).

If it turns out that this setup makes sense, we could think about adopting it in the other core plugins and swup itself. It would certainly make live easier for new contributors.

How to test...

...that unstaged files are being ignored

  1. Make some code ugly
  2. Don't stage the changes
  3. Run the pre-commit hook manually
    chmod +x .husky/pre-commit
    .husky/pre-commit

Expected Output in console:

→ No staged files found.

...that staged files are being formatted

  1. Make some code ugly
  2. Stage the file
  3. Try to commit the file

Expected output:

❯ git commit -m "test formatting of staged files"
✔ Preparing lint-staged...
✔ Running tasks for staged files...
✖ Prevented an empty git commit!
✔ Reverting to original state because of errors...
✔ Cleaning up temporary files...

  ⚠ lint-staged prevented an empty git commit.
  Use the --allow-empty option to continue, or check your task configuration

husky - pre-commit script failed (code 1)

Questions

Right now I'm re-using the swup package:format script. That script currently ignores markdown files. I guess there would be no harm in adding them? Also, css (for fade-theme, for example).

Alternative: Use prettier directly in lints-taged instead of the swup: package:format invocation:

export default {
-   '**/*.{js,jsx,mjs,cjs,ts,mts,css,scss,md}': ['swup package:format']
+   '**/*.{js,jsx,mjs,cjs,ts,mts,css,scss,md}': ['prettier --write']
};

That's probably the more flexible solution anyways...

So lint-staged will even prevent empty commits if formatting the code undos all changes to the staged files 🎉

Checks

github-actions[bot] commented 5 months ago

Playwright test results

passed  12 passed

Details

stats  12 tests across 1 suite
duration  35.3 seconds
commit  5f0abb6

daun commented 5 months ago

Looks great! My only concern with this type of setup is speed. The core repo freezes for at least a few seconds when committing, which is mildly annoying, but I assume that's either because of the size of the core codebase or the fact we're also tslinting and typechecking everything in the same step.

hirasso commented 5 months ago

@daun the problem with the setup we have in swup right now is that it checks the whole code base on each commit. That's exactly the selling point of lint-staged. Only check staged files, significantly speeding up the process 😎

daun commented 5 months ago

@hirasso Sounds fantastic!

hirasso commented 5 months ago

@daun sorry I requested your review too early. Could you please read the "Questions" section in the PR description? I'm not completely sure how to proceed there – maybe a quick call would be helpful so that we can test the behavior together and then decide what route we want to take.

daun commented 5 months ago

@hirasso Sorry, I missed the question part entirely :) I'd opt for sticking with the built-in package script. Right now, prettier and the script produce the same output, but whenever we're changing that, we'll have one more thing to debug when these two start getting in each other's way. Then again, I don't care too much and won't lose sleep over it.

hirasso commented 5 months ago

@daun seems like reusing swup package:format won't work 😢 ...the glob in that script overwrites the files passed by lint-staged as it seems, formatting the whole code base. The next best thing I could think of:

  1. Use prettier --write in lint-staged directly
  2. Add a new script swup format:lint-staged or the like to @swup/cli and use that in .lintstagedrc.js
  3. Add a .lintstagedrc.js to @swup/cli itself and re-export that from the plugins

Happy to walk you through the various options in a call.

daun commented 5 months ago

@hirasso Let's just use prettier directly via option 1 then — if it works, it works 🌝