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

Stop manipulating --public-url, and provide a default that works in most cases. #668

Closed
colons opened this issue Jan 8, 2024 · 6 comments
Labels
needs design This item needs design work

Comments

@colons
Copy link

colons commented Jan 8, 2024

In this comment, @ctron suggested that addressing the core problem that led to #320 with breaking changes might be a good idea. If that's allowable, then I have a simple proposal:

  • Make --public-url default to ..
    • For single-page applications hosted in a way where the URL structure maps directly to a filesystem, this will just work. I don't have data to back this up, but i feel like this is probably a common simple use case. It also won't break apps hosted on the root of a host, since there, . and / are equivalent.
  • Stop adding leading /s to explicitly-typed --public-urls.
    • If someone wants a leading /, they can type one. To insert one without asking is confusing and unnecessary.
@ctron
Copy link
Collaborator

ctron commented Jan 9, 2024

Without having checked, I believe an implementation of this should be rather easy. Having a PR ready for testing would allow people to check upfront and see what might be the impact of this change. I guess it would be a candidate for 0.19.x.

@ctron
Copy link
Collaborator

ctron commented Jan 26, 2024

Judging from #674, this issues feels a bit more complicated. Maybe let's go back to the drawing board for a second and try do find out what the best approach is.

From what I see, right now that information is used in two ways:

  1. To create links in the output file in the following pattern: {public_url}{resource}.
  2. To set up an axum router with a base URL

For the first case I think having a . as a default feels weird. That would lead to links like .image.svg. Which is wrong.

For the second case . (or any relative URL) won't work either.

I believe a relative URL will never work for serve. That would be ok, as we could just fail the startup. In any case, I believe that a trailing / needs to be added (unless there is one).

So we could change this into the following behavior:

  • The default public_url is empty
  • If it doesn't end with a slash, we'll always add one
  • If the command is serve and the URL is relative, we fail with an appropriate message

What do you think?

@ctron ctron added the needs design This item needs design work label Jan 26, 2024
@hmacias-avaya
Copy link
Contributor

I've been hit by this and went through some of the already open issues.

Reading from #697:

In a nutshell, when using serve, --public-url must be an absolute path. When using build, it can be relative.

it does sound like using the same exact option to have a different meaning depending on the operation (build vs serve) may be the reason for some of the pain experienced.

Perhaps there could be a serve_public_url and a public_url or any other names but have different flags for each.

Perhaps in some scenarios one can be derived from the other, but not in all scenarios.
In any case, if the user can manually specify both, we could have clear documentation and error messages if the value does not meet the expected criteria for each scenario.

@ctron
Copy link
Collaborator

ctron commented Feb 5, 2024

Nice summary, and absolutely true. Maybe having a good (helpful) error message in the serve case is already enough (in addition to not modifying the user provided value for public-url). So one can configure a default, and then override from the CLI? Adding another override in the [serve] section.

I guess all that it needs is to write it down. Think it through. And come up with a PR which can be tested for those various scenarios.

ctron added a commit to ctron/trunk that referenced this issue Feb 16, 2024
This change introduced the idea of the three different base URLs:

* The public base/common prefix, when generating links
* The "serve base", when serving content using `trunk serve`
* The "websocket base", where the auto-reload websocket connects to

The sane default still is to use an absolute --public-url, defaulting to
`/`. However, each of the URLs/bases can be overridden when necessary.

Closes: trunk-rs#668, trunk-rs#626
@ctron ctron closed this as completed in 391be74 Feb 16, 2024
@kang-sw
Copy link

kang-sw commented Mar 9, 2024

Nice change here ... But this surprised me today when my webpage suddenly broken with same command 😢

@ctron
Copy link
Collaborator

ctron commented Mar 11, 2024

I assume you've upgraded to trunk 0.19.0, which is a breaking change version.

WorldSEnder added a commit to WorldSEnder/yew that referenced this issue Aug 3, 2024
specifying an absolute URL as the public base is necessary since trunk 0.19
see also trunk-rs/trunk#668
WorldSEnder added a commit to yewstack/yew that referenced this issue Aug 3, 2024
specifying an absolute URL as the public base is necessary since trunk 0.19
see also trunk-rs/trunk#668
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs design This item needs design work
Projects
None yet
Development

No branches or pull requests

4 participants