Skip to content

🎨 Formatting Tooling: Oxfmt - #157

Merged
armando-navarro merged 17 commits into
FirebaseExtended:mainfrom
KingDarBoja:tools/formatter
Oct 29, 2026
Merged

armando-navarro merged 17 commits into
FirebaseExtended:mainfrom
KingDarBoja:tools/formatter

Conversation

@KingDarBoja

Copy link
Copy Markdown
Contributor
  • Added Oxfmt as modern formatting tool (it is Prettier-like).
  • Setup Pre-commit hooks to automatically format code using Husky.
  • Provide a GH Actions workflow for checking formatting. I believe the lint one within the test.yml should be separated as well but could be done in another pull request.

@KingDarBoja

Copy link
Copy Markdown
Contributor Author

@armando-navarro There we go!

@armando-navarro armando-navarro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: true sorts the scripts block alphabetically.
  • run-s build:** runs the matches in file order, so build:docs now runs before build:rollup has created dist/.
  • The fix I verified: "build": "run-s build:clean build:types build:rollup build:docs". Dropping sortScripts (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.yml lines 17 and 20: actions/checkout@v4 and actions/setup-node@v4. Please pin both to a commit SHA with the version in a comment, the way test.yml already pins actions/setup-java.
  • test.yml: 18 findings on uses: lines this PR never touched. The file is in the scan only because the reformat changed its quoting.
  • The simplest way out for test.yml is to leave it out of this PR: restore it from main and add "ignorePatterns": [".github/workflows/test.yml"] to .oxfmtrc.json. I checked that oxfmt --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-staged block in package.json ends up in the published dist/package.json, because rollup.config.mjs passes the whole root manifest through apart from scripts, devDependencies, files and private. Either move it to .lintstagedrc.json or add delete baseContents["lint-staged"] next to the files delete.
  • A line in CONTRIBUTING.md about yarn fmt and 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.md line 117 is missing from.
    • The user() and idToken() TypeScript examples in docs/auth.md are missing a brace (subscribe(u => { console.log(u); );).
    • In docs/database.md, the list() 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.

@KingDarBoja

Copy link
Copy Markdown
Contributor Author

@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 armando-navarro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@armando-navarro
armando-navarro merged commit 877b8a1 into FirebaseExtended:main Oct 29, 2026
19 checks passed
@KingDarBoja
KingDarBoja deleted the tools/formatter branch October 30, 2026 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants