Skip to content
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

feat: expose Args and new run_wasm_with_css_with_args #37

Closed
wants to merge 3 commits into from

Conversation

ErichDonGubler
Copy link

@ErichDonGubler ErichDonGubler commented Jun 6, 2023

It's currently impossible to embed cargo-run-wasm as a library unless one dedicates their entire command line interface to be forwarded to the library. This doesn't compose very well, sadly. We in wgpu would really like to be able to use this as a subcommand in a new cargo xtask flow (see gfx-rs/wgpu#3844 for more details).

More details on the current state of this PR: #37 (comment)

@ErichDonGubler
Copy link
Author

ErichDonGubler commented Jun 6, 2023

The current state of this PR is to expose a new constructor for Args that consumes a pico_args::Arguments structure. Thoughts about this:

  • It feels awkward to expose another crate's types as the interface for constructing an Args, but the current code
  • Current error handling is almost all panicking. Most of these panics should become a custom error representation, IMO, in order to have a really robust library.
  • In addition to exposing a constructor for Args, exposing the fields of the Args field might also be a good addition (but not required for the scope of this PR.

@ErichDonGubler
Copy link
Author

Oh, oof, sorry, accidental close. 😅

@rukai
Copy link
Owner

rukai commented Jun 8, 2023

Hi, thanks for the PR, I will review this and #36 holistically when I get the chance.
This is just a hobby project for me and I have a busy weekend this weekend, so most likely I will get to it on 17th-18th of June, but also possibly before.

Hopefully you are able to operate off a git fork for a while and this doesnt block your work.

@ErichDonGubler
Copy link
Author

@rukai: No problem at all. Wishing you well in the meantime! ☺️

@rukai
Copy link
Owner

rukai commented Jul 9, 2023

Finally got around to sitting and thinking about this.
This is what I've come up with to maintain the currently simple usage for the majority of users while allowing better integration with custom xtasks's like wgpu intends to do.

I'm thinking we could reimplement run_wasm_with_css something like this:

pub fn run_wasm_with_css(css: &str) {
    let args = match Args::from_env() {
        Ok(args) => args,
        Err(err) => {
            println!("{}\n\n{}", err, HELP);
            return;
        }
    };
    if args.help {
        println!("{}", HELP);
        return;
    }

  RunWasm::builder()
    .with_css(css)
    .with_host(args.host)
    .with_port(args.port)
    .with_profile(args.profile)
    .with_cargo_flags(args.cargo_flags) // args like --profile are inserted by RunWasm from with_profile and do not need to be specified here.
    // TODO: and every other flag we need
    .run()
}

This will maintain both run_wasm_with_csss API and runtime behavior.

From there we can expose RunWasm::builder() which will enable users to integrate cargo-run-wasm within their own xtask. They now get control over:

  • argument parsing - will need to bring their argument parsing library.
  • which parts of RunWasm to expose and which to hardcode - maybe they want to hardcode the port and never let the user set it.
  • generate their own help text - this is important within a custom xtask as the hardcoded help text in run_wasm_with_css would not make sense within a custom xtask.

Additionally this builder would end up being a suitable place to put the custom html logic desired by
#36
It would mean that winit would have to provide their own arg parsing, but I think that custom html is rare enough need that having to provide your own arg parsing is a reasonable cost.

Current error handling is almost all panicking. Most of these panics should become a custom error representation, IMO, in order to have a really robust library.

I am cautious about the need for proper error handling as this is simply a dev tool for previewing wasm and not suitable as part of a proper deployment stack. But! if we can achieve this without regressing on the usefulness of errors to a user who just unwraps the error then I am happy to land such a change.

I dont want to land this PR as is because exposing Args directly feels like a misstep.
If you want this design to be done urgently feel free to work on it yourself, otherwise I should be able to find a weekend to do it in within a month or two.

@ErichDonGubler
Copy link
Author

@rukai: I think a builder interface for arguments is a great direction. I'm happy to migrate over to it once it's ready! I'm not sure when I would be able to contribute it within the next month, but I might be able to author it in less than a couple of months. 🤔

RE: parsing arguments: I think it might be useful to offer an opaque API (perhaps over an iterator of OsStr?) over the current CLI args parsing to allow consumers to indirectly consume it, but I don't see a strong reason to expose the internals/data structure of argument parsing with a builder API.

Otherwise, it might be nice to make the CLI parsing feature-gated, too, to cut out another dependency. But that should be a separate issue, I suppose, as follow-up afterwards. :)

@rukai
Copy link
Owner

rukai commented Jul 28, 2023

Closing in favour of #40

@rukai rukai closed this Jul 28, 2023
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