RFC: Unify crate public APIs

[Boshen]

During flights without internet access, I was confused about all the inconsistencies from our crates' public APIs (oxc_parser, oxc_transformer, oxc_minifier etc.) - builder pattern or options pattern?

I'd like to research different Rust public API patterns, so we can have one less thing to do for v1.0.

[Boshen]

Research

builder pattern

https://github.com/colin-kiegel/rust-derive-builder

with pattern

https://docs.rs/schemars/latest/schemars/gen/struct.SchemaSettings.html#method.with

pub fn with(self, configure_fn: impl FnOnce(&mut Self)) -> Self

use schemars::gen::{SchemaGenerator, SchemaSettings};

let settings = SchemaSettings::default().with(|s| {
    s.option_nullable = true;
    s.option_add_null_type = false;
});
let gen = settings.into_generator();

[Boshen]

Proposal

What am looking for is consistency and future compatibility.

// Some tools return `Result`, some tools return recoverable errors.
// `OxcDiagnostic` can also contain warnings.
// When multiples tools are chained, errors can be chained as well.
pub struct ToolReturn {
  pub errors: Vec<OxcDiagnostic>,
  pub data: Data,
}

/// behavioral options
/// What should be the best pattern for breaking behavioral changes? e.g. Default value changed.
#{non_exhaustive]
#[derive(Default)]
pub struct ToolOptions {
  pub foo: Foo,
}

impl ToolOptions {
  pub fn with_foo(foo: Foo, yes: bool) -> Self {
    if yes {
      self.foo = foo;
    }
    self
  }
}

pub struct Tool {
  options: ToolOptions
}

impl Tool {
  pub fn new(essential_args: EssentialArgs) -> Self {
    Self { ... }
  }

  pub fn with_options(mut self, options: ToolOptions) -> Self {
    self.options = options;
    self
  }

  pub fn build(mut self, borrowed_arg: &BorrowedArg) -> ToolReturn {
    ToolReturn { ... }
  }
}


let options = ToolOptions::default().with_foo(foo, true);
let ret = Tool::new(essential_args).with_options(options).build();

[obi1kenobi]

Happy to dig in deeper next week, but a few smaller things I'd recommend keeping an eye on immediately:

• Make sure your builders either all take mut self or all take &mut self in their with_foo() methods. Either is fine, but mixing is very annoying.
• Builders with mut self mutation methods can sometimes be expensive, if the builder type is large and the compiler doesn't realize it can optimize all the intermediate moves away. &mut self can be safer from a perf maintainability perspective, and I know you care about perf deeply in oxc :)
• Consider making all your pub structs and enums #[non_exhaustive], and requiring that for all new types going forward. It's easy to add ahead of time, and very breaking to add later. (But remember it isn't breaking if a struct already had at least one non-pub field! cargo-semver-checks is your friend here, it gets this right 100% of the time.)
• For easy forward-compatibility, it's often easier to hide struct fields (non-pub) and have accessor methods marked #[inline] that give you a reference (mut or not, as needed). This will slightly slow down compilation time, but you'll be able to change the underlying representations without a breaking change.

[Boshen]

Oh yes, I forgot to mention another goal is to enable cargo-semver-checks after v1.0.0.

[obi1kenobi]

Woo! 🎉 Feel free to ping me anytime if you run into any issues with it, or just want a second pair of eyes on anything!

[milesj]

Just chiming in here, but I personally prefer builders that use &mut instead of immutable mut methods. Primarily because the latter makes calling methods in conditionals or blocks very awkward.

[hyf0]

#3485 Could be related. Hope builder pattern is optional.

Kind of hope the API is friendly for compiler to checkout breaking or unexpected changes. If the API requires users to use it with defensive programming in mind, it's probably not a good idea.

[Boshen]

We currently have an oxc crate for unifying all the sub-crates.

I plan to expose a Compiler API similar to rustc_driver and rustc_interface. The rustc API uses callbacks, closures and traits, I plan to use non of these patterns to make it easier for people coming from the JavaScript community.

The Compiler API will define the whole pipeline for source_text -> source_text transformation, i.e. parser -> semantic -> transform -> minify -> codegen.

Every step should have an before and after interceptor (plugin if you will).

When a user open the docs page of the oxc crate, they should be able to copy the example and customize.

Options API seems to be easier for this to work, compared to a builder pattern.

Here's the building block I propose, so I can start changing all the sub-crates to be consistent with this pattern:

struct CompilerOptions {
  parser: ParserOptions,
  semantic: Option<SemanticOptions>,
  transform: Option<TransformOptions>,
  minify: Option<MinifyOptions>,
  codegen: Option<CodegenOptions>
}

struct Compiler {
}

impl Compiler {
  pub fn new(options: CompilerOptions) {
  }
  
  pub fn run(self) {
  }
}

[Boshen]

I decided to not include any builder patterns in the public APIs, they are super confusing for new comers from JavaScript, who are used to passing objects with spread { foo: true, ...options }.

---

All the tools should conform to

// `OxcDiagnostic` can also contain warnings.
// When multiples tools are chained, errors can be chained as well.
pub struct ToolReturn {
  pub errors: Vec<OxcDiagnostic>,
  pub data: Data,
}

/// Behavioral options
#[derive(Default)]
pub struct ToolOptions {
  pub foo: bool,
}

pub struct Tool {
  options: ToolOptions
}

impl Tool {
  pub fn new(essential_args: EssentialArgs) -> Self {
    Self { ... }
  }

  pub fn with_options(mut self, options: ToolOptions) -> Self {
    self.options = options;
    self
  }

  pub fn build(mut self, borrowed_arg: &BorrowedArg) -> ToolReturn {
    ToolReturn { ... }
  }
}


let options = ToolOptions { foo: true, ..ToolOptions::default() };
let ret = Tool::new(essential_args).with_options(options).build();

[overlookmotel]

Boshen and I just discussed this on video chat. Plan is to standardize all the interfaces based on scheme above, and then to look at it again.

But just to write it down before I forget, my suggestion for API is this:

Tool is just a wrapper around options. All args ("essential" or not) are passed together into build, and then build creates the actual ToolImpl struct which is used for running the process.

The advantages are:

• Options can be pre-processed into the form in which they're needed in the actual process (parse options, validate etc).
• All data can be borrowed with same lifetime (so avoiding workarounds with Arc etc).

pub struct ToolReturn {
  pub errors: Vec<OxcDiagnostic>,
  pub data: Data,
}

#[derive(Default)]
pub struct ToolOptions {
  pub foo: bool,
}

pub struct Tool {
  options: ToolOptions
}

impl Tool {
  pub fn new() -> Self {
    Self { options: ToolOptions::default() }
  }

  // NB: Does not take `self`
  pub fn with_options(options: ToolOptions) -> Self {
    Self { options }
  }

  pub fn build(&self, arg1: &Arg1, arg2: &Arg2) -> ToolReturn {
    // Pre-process options here
    let foo_str = if self.options.foo { "yes" } else { "no" };
    let tool_impl = ToolImpl { foo_str, arg1, arg2 };
    tool_impl.run()
  }
}

struct ToolImpl<'a> {
  foo_str: &'a str,
  arg1: &'a Arg1,
  arg2: &'a Arg2,
}

impl<'a> ToolImpl<'a> {
  fn run() -> ToolReturn {
    // Do the stuff
  }
}

// User-facing API
// With default options
let ret = Tool::new().build(arg1, arg2);

// With options
let options = ToolOptions { foo: true, ..ToolOptions::default() };
let ret = Tool::with_options(options).build(arg1, arg2);

// Reusing same options
let options = ToolOptions { foo: true, ..ToolOptions::default() };
let tool = Tool::with_options(options);
let ret = tool.build(arg1, arg2);
let other_ret = tool.build(other_arg1, other_arg2);