trunk icon indicating copy to clipboard operation
trunk copied to clipboard

not add prefix / for public-url and default to ./

Open OmmyZhang opened this issue 1 year ago • 7 comments

This is an implementation for #668 .

I planned to add checking prefix '.' and 'https://' to avoid breaking changes. However, I see that there's already a lot of discussion and breaking changes are considered so I implemented as #668 .

Also fix docs of public_url which is wrong since 7acf4cdfa507c4e67d003cedfcb0c998ba76e1cf.

OmmyZhang avatar Jan 13 '24 06:01 OmmyZhang

I like this PR. But I think it would be the right time to also add some tests, to ensure this outcome is what is expected.

ctron avatar Jan 13 '24 15:01 ctron

I like this PR. But I think it would be the right time to also add some tests, to ensure this outcome is what is expected.

Thank you. Done.

OmmyZhang avatar Jan 15 '24 05:01 OmmyZhang

I think this looks good. I am just not sure when to merge it, as that would start the 0.19.x release cycle. I think there are a few other things would make sense to tackle in 0.19.x …

So please have a bit of patience with this one. But also feel free to ping if you think it takes too long :grin:

ctron avatar Jan 15 '24 07:01 ctron

@OmmyZhang I might merge a few other before merging this breaking change. Don't worry about rebasing, I will do that before/when I merge this.

ctron avatar Jan 17 '24 07:01 ctron

@NiklasEi can you post the console output of that panic? @OmmyZhang can you take a look?

ctron avatar Jan 18 '24 07:01 ctron

@NiklasEi can you post the console output of that panic? @OmmyZhang can you take a look?

thread 'tokio-runtime-worker' panicked at src/serve.rs:278:27:
Paths must start with a `/`
stack backtrace:
   0: rust_begin_unwind
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:72:14
   2: axum::routing::Router<S,B>::nest_service
   3: trunk::serve::router
   4: trunk::serve::ServeSystem::spawn_server::{{closure}}::{{closure}}
   5: trunk::serve::ServeSystem::run::{{closure}}::{{closure}}
   6: trunk::serve::ServeSystem::run::{{closure}}
   7: tokio::runtime::task::raw::poll
   8: tokio::runtime::scheduler::multi_thread::worker::Context::run_task
   9: tokio::runtime::task::raw::poll
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
[1]    41151 IOT instruction (core dumped)  RUST_BACKTRACE=1 trunk serve

NiklasEi avatar Jan 18 '24 12:01 NiklasEi

Sorry for overlooking the usage of public-url in trunk serve.

Now it should be fixed. Only use public-url as public_route when it starts with '/', and it seems that suffix '/' should not be removed.

OmmyZhang avatar Jan 19 '24 06:01 OmmyZhang

close as it should already be fixed in 391be74c74c320b3f01bf353d18bfc854d62bb85

OmmyZhang avatar Mar 06 '24 10:03 OmmyZhang