refactor: Add CreateGraphOptions, BuilderOptions

[bartlomieju]

Rewrite "create_graph", "create_code_graph" and "create_type_graph" to have fewer arguments. "CreateGraphOptions" struct was added, that has a "Default" trait implementation.

---

I find it quite hard and confusing to understand what's going on because of create_graph having 8 arguments, while most of them are None.

Ideally we could have shorthand method for CreateGraphOptions like:

CreateGraphOptions {
   roots: vec![spec.to_string()],
   loader: &mut some_loader,
   ..Default::default()
}

however, because of roots and loader being required it's not possible to derive Default trait. Maybe we could use CreateGraphOptions::default(roots, loader) method?

[nayeemrmn]

however, because of roots and loader being required it's not possible to derive Default trait. Maybe we could use CreateGraphOptions::default(roots, loader) method?

I think normally those would be excluded from the options struct and kept as positional args.

[bartlomieju]

@nayeemrmn thanks for suggestion, totally obvious but I missed it. I think this is a lot cleaner.

[bartlomieju]

You need to rebuild the WASM and ensure whatever impacts on the JS API are handled properly.

Rebuilt WASM, it seems JS APIs are not impacted by this change (because it uses Builder directly)

[bartlomieju]

Sure, I'll do that.

[bartlomieju]

@kitsonk do you have any objections to moving arguments of Builder::build() into Builder::new()?

So instead of:

let builder = Builder::new(
  roots,
  loader,
  source_parser,
  BuilderOptions {
    is_dynamic_root: options.is_dynamic,
    maybe_resolver: options.maybe_resolver,
    maybe_locker: options.maybe_locker,
    maybe_reporter: options.maybe_reporter,
  }
);
builder.build(build_kind, options.maybe_imports).await

we would do:

let builder = Builder::new(
  build_kind,
  loader,
  source_parser,
  BuilderOptions {
    is_dynamic_root: options.is_dynamic,
    maybe_resolver: options.maybe_resolver,
    maybe_locker: options.maybe_locker,
    maybe_reporter: options.maybe_reporter,
    maybe_imports: options.maybe_imports
  }
);
builder.build().await

[kitsonk]

@kitsonk do you have any objections to moving arguments of Builder::build() into Builder::new()?

There was some reason at one point, no specific objection, but it maybe worth taking a look at how it is consumed in CLI, as IIRC there were some challenges in the flow there that lead to that separation of options, but it maybe just me being crazy.

[dsherret]

I think we can close this one because it's already done in a separate PR, but please re-open if that's not the case.