From 4767926ea43adce4cf0394f1ea0903a474c885da Mon Sep 17 00:00:00 2001 From: leaysgur <6259812+leaysgur@users.noreply.github.com> Date: Thu, 4 Dec 2025 10:03:50 +0000 Subject: [PATCH] feat(oxfmt): Prepare non-js/ts file support with prettier (#16480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - [x] Implement - walk.rs: Only collect files that need formatting - This is needed to ignore binaries like `.png`, etc - support.rs: Process to determine eligibility based on `path` - Currently still only JS/TS - Plan to add more supported files in the next PR - prettier-proxy.js: Changed the signature for calling Prettier from `file_name` to `parser` specification - Allows skipping `inferParser` - [x] Decide how to address problems below - 👉🏻 Maintain our own target list, same as prettier, biome - Bonus: specifying parser can skip `inferParser` step in prettier - For plugins, read config file beforehand in JS... - 👉🏻 Always ignore unknown files - It means we don't implement `--ignore-unknown` - Only supports JS/TS + Prettier default supported + Defined by plugins
Problems - Let Prettier handle all non-JS/TS files is fine - But there are still 2 kinds of unsupported files - Non binary unknown format like `.svelte`, etc - Binary format like `.png`, etc - For former: Prettier throws `UndefinedParsererror` - We need to detect and handle this with `--ignore-unknown` or always ignore - For latter - Prettier also throws if `prettier ./src/public/foo.png` - But NOT for `prettier ./src` 🤯 - Easiest solution is just let handle to prettier but it is really slow - Because it means: read file, call prettier, check result for every single file in the repo - Non-binary detection may be good idea, but it still requires reading file
Also still doesn't affect existing use cases. --- apps/oxfmt/src-js/bindings.d.ts | 2 +- apps/oxfmt/src-js/prettier-proxy.ts | 9 +-- apps/oxfmt/src/format.rs | 6 +- apps/oxfmt/src/lib.rs | 1 + apps/oxfmt/src/main_napi.rs | 2 +- .../prettier_plugins/external_formatter.rs | 22 ++--- apps/oxfmt/src/service.rs | 80 ++++++++++--------- apps/oxfmt/src/support.rs | 46 +++++++++++ apps/oxfmt/src/walk.rs | 53 ++++++------ 9 files changed, 136 insertions(+), 85 deletions(-) create mode 100644 apps/oxfmt/src/support.rs diff --git a/apps/oxfmt/src-js/bindings.d.ts b/apps/oxfmt/src-js/bindings.d.ts index 7b2758eba34c9..3994a3a271066 100644 --- a/apps/oxfmt/src-js/bindings.d.ts +++ b/apps/oxfmt/src-js/bindings.d.ts @@ -10,4 +10,4 @@ * * Returns `true` if formatting succeeded without errors, `false` otherwise. */ -export declare function format(args: Array, formatEmbeddedCb: (tagName: string, code: string) => Promise, formatFileCb: (fileName: string, code: string) => Promise): Promise +export declare function format(args: Array, formatEmbeddedCb: (tagName: string, code: string) => Promise, formatFileCb: (parserName: string, code: string) => Promise): Promise diff --git a/apps/oxfmt/src-js/prettier-proxy.ts b/apps/oxfmt/src-js/prettier-proxy.ts index cee4db10edd13..e062ac9e7e460 100644 --- a/apps/oxfmt/src-js/prettier-proxy.ts +++ b/apps/oxfmt/src-js/prettier-proxy.ts @@ -66,20 +66,17 @@ export async function formatEmbeddedCode(tagName: string, code: string): Promise /** * Format whole file content using Prettier. * NOTE: Called from Rust via NAPI ThreadsafeFunction with FnArgs - * @param fileName - The file name (used to infer parser) + * @param parserName - The parser name * @param code - The code to format * @returns Formatted code */ -export async function formatFile(fileName: string, code: string): Promise { +export async function formatFile(parserName: string, code: string): Promise { if (!prettierCache) { prettierCache = await import("prettier"); } - // TODO: Tweak parser for `tsconfig.json` with `jsonc` parser? - return prettierCache.format(code, { - // Let Prettier infer the parser - filepath: fileName, + parser: parserName, // TODO: Read config }); } diff --git a/apps/oxfmt/src/format.rs b/apps/oxfmt/src/format.rs index 2075f42196113..b6a35fb0a59b6 100644 --- a/apps/oxfmt/src/format.rs +++ b/apps/oxfmt/src/format.rs @@ -61,7 +61,7 @@ impl FormatRunner { // NOTE: Currently, we only load single config file. // - from `--config` if specified // - else, search nearest for the nearest `.oxfmtrc.json` from cwd upwards - let config = match load_config(&cwd, basic_options.config.as_ref()) { + let config = match load_config(&cwd, basic_options.config.as_deref()) { Ok(config) => config, Err(err) => { print_and_flush(stderr, &format!("Failed to load configuration file.\n{err}\n")); @@ -220,11 +220,11 @@ impl FormatRunner { /// Returns error if: /// - Config file is specified but not found or invalid /// - Config file parsing fails -fn load_config(cwd: &Path, config_path: Option<&PathBuf>) -> Result { +fn load_config(cwd: &Path, config_path: Option<&Path>) -> Result { let config_path = if let Some(config_path) = config_path { // If `--config` is explicitly specified, use that path Some(if config_path.is_absolute() { - PathBuf::from(config_path) + config_path.to_path_buf() } else { cwd.join(config_path) }) diff --git a/apps/oxfmt/src/lib.rs b/apps/oxfmt/src/lib.rs index 8e99594d54fa9..70524cb82b622 100644 --- a/apps/oxfmt/src/lib.rs +++ b/apps/oxfmt/src/lib.rs @@ -5,6 +5,7 @@ mod lsp; mod reporter; mod result; mod service; +mod support; mod walk; // Public re-exports for use in main.rs and lib consumers diff --git a/apps/oxfmt/src/main_napi.rs b/apps/oxfmt/src/main_napi.rs index d58e6b30af503..7c857111f957b 100644 --- a/apps/oxfmt/src/main_napi.rs +++ b/apps/oxfmt/src/main_napi.rs @@ -33,7 +33,7 @@ pub async fn format( args: Vec, #[napi(ts_arg_type = "(tagName: string, code: string) => Promise")] format_embedded_cb: JsFormatEmbeddedCb, - #[napi(ts_arg_type = "(fileName: string, code: string) => Promise")] + #[napi(ts_arg_type = "(parserName: string, code: string) => Promise")] format_file_cb: JsFormatFileCb, ) -> bool { format_impl(args, format_embedded_cb, format_file_cb).await.report() == ExitCode::SUCCESS diff --git a/apps/oxfmt/src/prettier_plugins/external_formatter.rs b/apps/oxfmt/src/prettier_plugins/external_formatter.rs index e02626467f717..0a72b5da51dcd 100644 --- a/apps/oxfmt/src/prettier_plugins/external_formatter.rs +++ b/apps/oxfmt/src/prettier_plugins/external_formatter.rs @@ -23,10 +23,10 @@ pub type JsFormatEmbeddedCb = ThreadsafeFunction< >; /// Type alias for the callback function signature. -/// Takes (tag_name, code) as separate arguments and returns formatted code. +/// Takes (parser_name, code) as separate arguments and returns formatted code. pub type JsFormatFileCb = ThreadsafeFunction< // Input arguments - FnArgs<(String, String)>, // (file_name, code) as separate arguments + FnArgs<(String, String)>, // (parser_name, code) as separate arguments // Return type (what JS function returns) Promise, // Arguments (repeated) @@ -38,7 +38,7 @@ pub type JsFormatFileCb = ThreadsafeFunction< >; /// Callback function type for formatting files. -/// Takes (file_name, code) and returns formatted code or an error. +/// Takes (parser_name, code) and returns formatted code or an error. type FileFormatterCallback = Arc Result + Send + Sync>; /// External formatter that wraps a JS callback. @@ -73,8 +73,8 @@ impl ExternalFormatter { } /// Format non-js file using the JS callback. - pub fn format_file(&self, file_name: &str, code: &str) -> Result { - (self.format_file)(file_name, code) + pub fn format_file(&self, parser_name: &str, code: &str) -> Result { + (self.format_file)(parser_name, code) } } @@ -101,19 +101,19 @@ fn wrap_format_embedded(cb: JsFormatEmbeddedCb) -> EmbeddedFormatterCallback { /// Wrap JS `formatFile` callback as a normal Rust function. fn wrap_format_file(cb: JsFormatFileCb) -> EmbeddedFormatterCallback { - Arc::new(move |file_name: &str, code: &str| { + Arc::new(move |parser_name: &str, code: &str| { block_on(async { let status = - cb.call_async(FnArgs::from((file_name.to_string(), code.to_string()))).await; + cb.call_async(FnArgs::from((parser_name.to_string(), code.to_string()))).await; match status { Ok(promise) => match promise.await { Ok(formatted_code) => Ok(formatted_code), - Err(err) => { - Err(format!("JS formatter promise rejected for file '{file_name}': {err}")) - } + Err(err) => Err(format!( + "JS formatter promise rejected for file '{parser_name}': {err}" + )), }, Err(err) => Err(format!( - "Failed to call JS formatting callback for file '{file_name}': {err}" + "Failed to call JS formatting callback for file '{parser_name}': {err}" )), } }) diff --git a/apps/oxfmt/src/service.rs b/apps/oxfmt/src/service.rs index 3396498c4c488..c69df153c5376 100644 --- a/apps/oxfmt/src/service.rs +++ b/apps/oxfmt/src/service.rs @@ -5,13 +5,16 @@ use rayon::prelude::*; use oxc_allocator::AllocatorPool; use oxc_diagnostics::{DiagnosticSender, DiagnosticService, Error, OxcDiagnostic}; -use oxc_formatter::{ - FormatOptions, Formatter, enable_jsx_source_type, get_parse_options, get_supported_source_type, -}; +use oxc_formatter::{FormatOptions, Formatter, enable_jsx_source_type, get_parse_options}; use oxc_parser::Parser; use oxc_span::SourceType; -use crate::{command::OutputOptions, walk::WalkEntry}; +use crate::{command::OutputOptions, support::FormatFileSource}; + +enum FormatFileResult { + Success { is_changed: bool, code: String }, + Error(Vec), +} pub enum SuccessResult { Changed(String), @@ -60,23 +63,23 @@ impl FormatService { /// Process entries as they are received from the channel pub fn run_streaming( &self, - rx_entry: mpsc::Receiver, + rx_entry: mpsc::Receiver, tx_error: &DiagnosticSender, tx_success: &mpsc::Sender, ) { rx_entry.into_iter().par_bridge().for_each(|entry| { let start_time = Instant::now(); - let path = &entry.path; let (code, is_changed) = match self.format_file(&entry) { - Ok(res) => res, - Err(diagnostics) => { + FormatFileResult::Success { is_changed, code } => (code, is_changed), + FormatFileResult::Error(diagnostics) => { tx_error.send(diagnostics).unwrap(); return; } }; let elapsed = start_time.elapsed(); + let path = &entry.path(); // Write back if needed if matches!(self.output_options, OutputOptions::Write) && is_changed { @@ -110,11 +113,13 @@ impl FormatService { }); } - fn format_file(&self, entry: &WalkEntry) -> Result<(String, bool), Vec> { - let path = &entry.path; + // NOTE: This may be `pub` for LSP usage in the future + fn format_file(&self, entry: &FormatFileSource) -> FormatFileResult { + let path = entry.path(); let Ok(source_text) = read_to_string(path) else { - // This happens if `.ts` for MPEG-TS binary is attempted to be formatted + // This happens if binary file is attempted to be formatted + // e.g. `.ts` for MPEG-TS video file let diagnostics = DiagnosticService::wrap_diagnostics( self.cwd.clone(), path, @@ -124,32 +129,30 @@ impl FormatService { .with_help("This may be due to the file being a binary or inaccessible."), ], ); - return Err(diagnostics); + return FormatFileResult::Error(diagnostics); }; - let code = match get_supported_source_type(path.as_path()) { - Some(source_type) => self.format_by_oxc_formatter(entry, &source_text, source_type)?, + match entry { + FormatFileSource::OxcFormatter { path, source_type } => { + self.format_by_oxc_formatter(&source_text, path, *source_type) + } #[cfg(feature = "napi")] - None => self.format_by_external_formatter(entry, &source_text)?, + FormatFileSource::ExternalFormatter { path, parser_name } => { + self.format_by_external_formatter(&source_text, path, parser_name) + } #[cfg(not(feature = "napi"))] - None => unreachable!( - "If `napi` feature is disabled, non-supported entry should not be passed: {}", - path.display() - ), - }; - - let is_changed = source_text != code; - - Ok((code, is_changed)) + FormatFileSource::ExternalFormatter { .. } => { + unreachable!("If `napi` feature is disabled, this should not be passed here",) + } + } } fn format_by_oxc_formatter( &self, - entry: &WalkEntry, source_text: &str, + path: &Path, source_type: SourceType, - ) -> Result> { - let path = &entry.path; + ) -> FormatFileResult { let source_type = enable_jsx_source_type(source_type); let allocator = self.allocator_pool.get(); @@ -163,7 +166,7 @@ impl FormatService { source_text, ret.errors, ); - return Err(diagnostics); + return FormatFileResult::Error(diagnostics); } let base_formatter = Formatter::new(&allocator, self.format_options.clone()); @@ -196,7 +199,7 @@ impl FormatService { path.display() ))], ); - return Err(diagnostics); + return FormatFileResult::Error(diagnostics); } }; @@ -208,27 +211,24 @@ impl FormatService { } } - Ok(code) + FormatFileResult::Success { is_changed: source_text != code, code } } #[cfg(feature = "napi")] fn format_by_external_formatter( &self, - entry: &WalkEntry, source_text: &str, - ) -> Result> { - let path = &entry.path; - let file_name = path.file_name().expect("Path must have a file name").to_string_lossy(); - + path: &Path, + parser_name: &str, + ) -> FormatFileResult { let code = match self .external_formatter .as_ref() .expect("`external_formatter` must exist when `napi` feature is enabled") - .format_file(&file_name, source_text) + .format_file(parser_name, source_text) { Ok(code) => code, Err(err) => { - // TODO: Need to handle `UndefinedParserError` or not let diagnostics = DiagnosticService::wrap_diagnostics( self.cwd.clone(), path, @@ -238,14 +238,16 @@ impl FormatService { path.display() ))], ); - return Err(diagnostics); + return FormatFileResult::Error(diagnostics); } }; - Ok(code) + FormatFileResult::Success { is_changed: source_text != code, code } } } +// --- + fn read_to_string(path: &Path) -> io::Result { // `simdutf8` is faster than `std::str::from_utf8` which `fs::read_to_string` uses internally let bytes = fs::read(path)?; diff --git a/apps/oxfmt/src/support.rs b/apps/oxfmt/src/support.rs new file mode 100644 index 0000000000000..dea262d0fd951 --- /dev/null +++ b/apps/oxfmt/src/support.rs @@ -0,0 +1,46 @@ +use std::path::{Path, PathBuf}; + +use oxc_formatter::get_supported_source_type; +use oxc_span::SourceType; + +pub enum FormatFileSource { + OxcFormatter { + path: PathBuf, + source_type: SourceType, + }, + #[expect(dead_code)] + ExternalFormatter { + path: PathBuf, + parser_name: String, + }, +} + +impl TryFrom<&Path> for FormatFileSource { + type Error = (); + + fn try_from(path: &Path) -> Result { + // TODO: This logic should(can) move to this file, after LSP support is also moved here. + if let Some(source_type) = get_supported_source_type(path) { + return Ok(Self::OxcFormatter { path: path.to_path_buf(), source_type }); + } + + // TODO: Support more files with `ExternalFormatter` + // - JSON + // - HTML(include .vue) + // - CSS + // - GraphQL + // - Markdown + // - YAML + // - Handlebars + + Err(()) + } +} + +impl FormatFileSource { + pub fn path(&self) -> &Path { + match self { + Self::OxcFormatter { path, .. } | Self::ExternalFormatter { path, .. } => path, + } + } +} diff --git a/apps/oxfmt/src/walk.rs b/apps/oxfmt/src/walk.rs index a5ac81f8a5ccf..2054d814bf7f8 100644 --- a/apps/oxfmt/src/walk.rs +++ b/apps/oxfmt/src/walk.rs @@ -5,7 +5,7 @@ use std::{ use ignore::{gitignore::GitignoreBuilder, overrides::OverrideBuilder}; -use oxc_formatter::get_supported_source_type; +use crate::support::FormatFileSource; pub struct Walk { inner: ignore::WalkParallel, @@ -13,7 +13,7 @@ pub struct Walk { impl Walk { pub fn build( - cwd: &PathBuf, + cwd: &Path, paths: &[PathBuf], ignore_paths: &[PathBuf], with_node_modules: bool, @@ -65,11 +65,9 @@ impl Walk { // NOTE: If return `false` here, it will not be `visit()`ed at all inner.filter_entry(move |entry| { - // Skip stdin for now let Some(file_type) = entry.file_type() else { return false; }; - let is_dir = file_type.is_dir(); if is_dir { @@ -77,7 +75,7 @@ impl Walk { // it means we want to include hidden files and directories. // However, we (and also Prettier) still skip traversing certain directories. // https://prettier.io/docs/ignore#ignoring-files-prettierignore - let is_skipped_dir = { + let is_ignored_dir = { let dir_name = entry.file_name(); dir_name == ".git" || dir_name == ".jj" @@ -86,7 +84,7 @@ impl Walk { || dir_name == ".hg" || (!with_node_modules && dir_name == "node_modules") }; - if is_skipped_dir { + if is_ignored_dir { return false; } } @@ -97,10 +95,10 @@ impl Walk { return false; } - // NOTE: We can also check `get_supported_source_type()` here to skip. - // But we want to pass parsed `SourceType` to `FormatService`, - // so we do it later in the visitor instead. - + // NOTE: In addition to ignoring based on ignore files and patterns here, + // we also apply extra filtering in the visitor `visit()` below. + // We need to return `bool` for `filter_entry()` here, + // but we don't want to duplicate logic in the visitor again. true }); @@ -121,8 +119,8 @@ impl Walk { } /// Stream entries through a channel as they are discovered - pub fn stream_entries(self) -> mpsc::Receiver { - let (sender, receiver) = mpsc::channel::(); + pub fn stream_entries(self) -> mpsc::Receiver { + let (sender, receiver) = mpsc::channel::(); // Spawn the walk operation in a separate thread rayon::spawn(move || { @@ -199,12 +197,8 @@ fn load_ignore_paths(cwd: &Path, ignore_paths: &[PathBuf]) -> Vec { // --- -pub struct WalkEntry { - pub path: PathBuf, -} - struct WalkBuilder { - sender: mpsc::Sender, + sender: mpsc::Sender, } impl<'s> ignore::ParallelVisitorBuilder<'s> for WalkBuilder { @@ -214,7 +208,7 @@ impl<'s> ignore::ParallelVisitorBuilder<'s> for WalkBuilder { } struct WalkVisitor { - sender: mpsc::Sender, + sender: mpsc::Sender, } impl ignore::ParallelVisitor for WalkVisitor { @@ -227,14 +221,25 @@ impl ignore::ParallelVisitor for WalkVisitor { // Use `is_file()` to detect symlinks to the directory named `.js` #[expect(clippy::filetype_is_file)] - if file_type.is_file() - // TODO: Remove it for napi, keep it for not(napi) - && get_supported_source_type(entry.path()).is_some() - { - let walk_entry = WalkEntry { path: entry.path().to_path_buf() }; + if file_type.is_file() { + let path = entry.path(); + // TODO: Determine this file should be handled or NOT + // Tier 1 = `.js`, `.tsx`, etc: JS/TS files supported by `oxc_formatter` + // Tier 2 = `.html`, `.json`, etc: Other files supported by Prettier + // (Tier 3 = `.astro`, `.svelte`, etc: Other files supported by Prettier plugins) + // Tier 4 = everything else: Not handled + let Ok(format_file_source) = FormatFileSource::try_from(path) else { + return ignore::WalkState::Continue; + }; + + #[cfg(not(feature = "napi"))] + if matches!(format_file_source, FormatFileSource::ExternalFormatter { .. }) { + return ignore::WalkState::Continue; + } + // Send each entry immediately through the channel // If send fails, the receiver has been dropped, so stop walking - if self.sender.send(walk_entry).is_err() { + if self.sender.send(format_file_source).is_err() { return ignore::WalkState::Quit; } }