-
-
Notifications
You must be signed in to change notification settings - Fork 763
fix(oxfmt): Report exitCode correctly
#16770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
exitCode correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change generally aligns exit code reporting between Rust and JS, but the new tuple-based contract has a few correctness and robustness gaps. cli.ts can accidentally assign process.exitCode = undefined if exitCode is missing, and the N-API arg-parse error path currently collapses non-zero parser exit codes into 1, which may undermine “report exitCode correctly”. The .d.ts type is looser than runtime (null), and returning mode as free-form String increases the chance of drift/typos across the FFI boundary.
Additional notes (1)
- Maintainability |
apps/oxfmt/src/main_napi.rs:28-28
run_clireturns(String, Option<u8>)and constructs mode strings via"cli".to_string()etc. This is error-prone (typos become runtime bugs) and pushes a looser type across the FFI boundary. Since the JS side branches on a closed set of modes, it’s better to encode that as a Rust enum exported to N-API (or at least a&'static str/Cow<'static, str>that can only be one of the known values).
Even if you keep a string, consider centralizing the literals to avoid drift between Rust docs, Rust return values, and TS declaration.
Summary of changes
What changed
-
N-API surface changed from boolean to structured result
- Replaced exported binding
formatwithrunCli/run_cli. - Updated TypeScript declarations to return a tuple
[mode, exitCode]wheremodeis one of"cli" | "lsp" | "init"andexitCodeis set only for CLI.
- Replaced exported binding
-
JS CLI now delegates mode parsing to Rust (
apps/oxfmt/src-js/cli.ts)- Calls
runCli(args, ...)and switches onmode. - Sets
process.exitCodebased on Rust-providedexitCodefor"cli"mode. - Reintroduces
--inithandling, but only after Rust indicates"init"mode.
- Calls
-
Rust exit code handling is centralized (
apps/oxfmt/src/cli/result.rs)- Added
CliRunResult::exit_code()returningu8. Termination::report()now usesself.exit_code().
- Added
-
N-API entry point now returns mode + exit code (
apps/oxfmt/src/main_napi.rs)format(...) -> boolreplaced withrun_cli(...) -> (String, Option<u8>).- On arg-parse failures, returns
("cli", Some(0|1))after printing message. Mode::Initnow returns("init", None)instead ofunreachable!.
-
Tests updated
- Snapshots updated to expect exit code
2for “no files found/expected at least one target file” scenarios.
- Snapshots updated to expect exit code
f91d0f1 to
2bbdef9
Compare
fb899cf to
2577814
Compare
2bbdef9 to
aab3331
Compare
Merge activity
|
|
Too fast approve! I was fixing CI...😗 |
aab3331 to
6a9424d
Compare
Previously, - `oxfmt(napi) not-exists.js` returns `exitCode: 1` - `oxfmt(rust) not-exists.js` returns `exitCode: 2` = Expected To fix this, I updated napi callback return value from `bool`. Additionally, I realized that by returning the processing `Mode` simultaneously, Rust can handle the CLI arguments in better way. Like `--init --help`, which now shows help. And finally changed napi binding name from `format()` to `runCli()` for the future Node API.
6a9424d to
bc2e0f8
Compare

Previously,
oxfmt(napi) not-exists.jsreturnsexitCode: 1oxfmt(rust) not-exists.jsreturnsexitCode: 2= ExpectedTo fix this, I updated napi callback return value from
bool.Additionally, I realized that by returning the processing
Modesimultaneously, Rust can handle the CLI arguments in better way. Like--init --help, which now shows help.And finally changed napi binding name from
format()torunCli()for the future Node API.