Repository navigation
🎨 Formatting Tooling: Oxfmt - #157
Conversation
- Setup pre-commit hooks using husky.
|
@armando-navarro There we go! |
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks for sending this as its own PR, as we discussed on #123. I ran your oxfmt config against current main and it reproduces your reformat commit byte for byte. Two things I would ask for before this can go in, and a few optional ones.
Build order (blocking)
yarn build fails on this branch. Your fork's Build job on bf16604 shows it: https://github.com/KingDarBoja/rxfire/actions/runs/40878175419
sortScripts: truesorts thescriptsblock alphabetically.run-s build:**runs the matches in file order, sobuild:docsnow runs beforebuild:rolluphas createddist/.- The fix I verified:
"build": "run-s build:clean build:types build:rollup build:docs". DroppingsortScripts(off by default in oxfmt) also restores the order, but the explicit list holds under any future sort, so I would do that either way.
The workflow security scan (blocking)
The GitHub Actions Scan check fails on this PR with 20 unpinned-action findings in two files. It covers every workflow file a PR changes, in full.
format.ymllines 17 and 20:actions/checkout@v4andactions/setup-node@v4. Please pin both to a commit SHA with the version in a comment, the waytest.ymlalready pinsactions/setup-java.test.yml: 18 findings onuses:lines this PR never touched. The file is in the scan only because the reformat changed its quoting.- The simplest way out for
test.ymlis to leave it out of this PR: restore it frommainand add"ignorePatterns": [".github/workflows/test.yml"]to.oxfmtrc.json. I checked thatoxfmt --check .stays green with that and that the lint-staged task then leaves the file alone. - Pinning those 18 references can be its own PR.
Optional
- The
lint-stagedblock inpackage.jsonends up in the publisheddist/package.json, becauserollup.config.mjspasses the whole root manifest through apart fromscripts,devDependencies,filesandprivate. Either move it to.lintstagedrc.jsonor adddelete baseContents["lint-staged"]next to thefilesdelete. - A line in
CONTRIBUTING.mdaboutyarn fmtand the pre-commit hook would save the next contributor a surprise when the hook rewrites their files. - oxfmt skips a code block it cannot parse, so a few doc examples stayed single-quoted next to reformatted ones. They are wrong as examples anyway:
README.mdline 117 is missingfrom.- The
user()andidToken()TypeScript examples indocs/auth.mdare missing a brace (subscribe(u => { console.log(u); );). - In
docs/database.md, thelist()example never closes its.pipe(, and lines 142 and 190 have...change.snapshot.val();inside an object literal.
One question rather than an ask. On #123 I had Prettier in mind. oxfmt is at 0.75 with a release most weeks, so each bump may reformat the tree and fail fmt:check until someone re-applies it. Was there a reason you preferred it? I am open to it, I just want to go in with eyes open.
Also on the config: main's code is single-quoted today, and most Angular and Firebase apps I see are too. Would you be open to singleQuote: true? On main that gives a smaller reformat and keeps the README and docs examples single-quoted.
If I have misread anything here, tell me and I will take another look.
|
@armando-navarro I do agree with most feedback points and regarding the choice between Prettier and Oxfmt, I wanted to avoid a lot of Prettier config overhead as it would require extra plugins (and therefore more dependencies to update in the long term) in order to enhance the formatting experience. With Oxfmt, several plugins are already supported out-of-the-box as you can check those at the official docs Oxfmt - Beyond Prettier. Also it works well even if this codebase is kinda "small" and offer a Prettier-like experience. And yeah, I didn't take into account the fact that it's in a beta version (currently 0.75) so it's a valid concern. |
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. Everything from the first round is in and looks good.
On oxfmt vs Prettier, understood, and the plugin-free setup is a fair point. We'll accept the 0.x release pace knowingly, keeping an eye out for a version bump reformatting the tree. Thanks for taking the optional items too.
Approving.
test.ymlshould be separated as well but could be done in another pull request.