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: allow more control over the different base URLs #717

Merged
merged 7 commits into from
Feb 16, 2024

Conversation

ctron
Copy link
Collaborator

@ctron ctron commented Feb 16, 2024

No description provided.

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
Copy link
Collaborator Author

ctron commented Feb 16, 2024

@allan2 @colons @hmacias-avaya here's a proposal for dealing with this. It would be great if any of you could take a look if this solves your problems.

@ctron ctron merged commit 5156858 into trunk-rs:main Feb 16, 2024
43 checks passed
@ctron ctron deleted the feature/urls_1 branch February 16, 2024 16:55
@hmacias-avaya
Copy link
Contributor

I just tried it; the public-url seems to help, at the cost of having the user specify this.

My scenario:

  • browser opens http://host/my_app/index.html
  • reverse proxy sends all requests originally to "/my_app/*" into my server (trunk during development)
  • trunk receives http://host/index.html. Whichever resources referenced by this file need to "resolve" according to the reverse proxy rules.

I ran trunk using trunk serve --public-url /my-app --serve-base /

When I send this request and see what index.html contains, I can see /my_app/ prepended everywhere which makes the browser requests for all the other resources (wasm, js, etc) succeed.

For example

<link rel="preload" href="/my-app/application.wasm" ... >

So, so far, so good.

If I send a request to some other subpath, for example http://host/something/index.html I see trunk still sends OK with the same content with all resources pointing to /my_app/.

Perhaps this is not a valid scenario (I used something in the browser url which is not what I passed trunk as public-url), but I wonder if we really need to tell trunk what the public-url needs to be.

If trunk was using paths relative to the resource requested, trunk itself wouldn't care about the public url when serving.
For the following 3 urls:

trunk could return the same content using relative paths:

<link rel="preload" href="./application.wasm" ... >

and this would mean the user needs to specify one less argument over the cli. Trunk is completely unaware of what the reverse proxy is doing and doesn't need to know any of the request manipulations that happen before it (as long as relative urls are used).

Again I'm likely missing something else but if there is a way to achieve the same with less configuration options I'd go for that.

@hmacias-avaya
Copy link
Contributor

...if I run with trunk serve --public-url ./ --serve-base / I do get all resources relative:

<link rel="preload" href="./application.wasm" ... >

which makes this work regardless of the subpath used, which is good (and why I think ./ should be the default).
On the the other hand if I run trunk serve --serve-base / this will not work with a reverse proxy manipulating the urls as the previous scenario.

So, all in all, the ./ seems to cover the default scenario / and several more, but I'm not sure I see which scenarios is breaking. If it's not breaking anything and is covering more scenarios, I'd suggest using ./ as the default instead.

@ctron
Copy link
Collaborator Author

ctron commented Feb 17, 2024

Thanks for testing! And I think you just proved that what I had in my mind worked:

at the cost of having the user specify this.

I would rephrase this as: The user has the ability now to specify this

I think ./ should be the default

That may be the case for you, but not necessarily for everyone else. Using ./ as a default will mean, when using History API routers, that you start out loading https://foo/bar/app.wasm. When you reload from a sub-page, you will get https://foo/bar/path/to/sub/app.wasm. And while trunk will give you the index.html in such a case, to support SPAs, it won't give you any other resource. As it simply cannot tell what resource the application had in mind. And the same would be true for all/most other HTTP servers hosting the static SPA files.

So unless we find a good way to deal with this, not only with trunk, but also other HTTP servers hosting such applications, I am not keen on replacing partially working solution with another partially working solution. I think today's behavior is limited, but understood. The PR removed the limitation, but keep the current understanding on the behavior.

@NiklasEi
Copy link

NiklasEi commented Feb 23, 2024

Thanks for this! My use case (basically a SPA, that can be hosted at random paths) now works with the alpha.3 release without breaking trunk serve.
Is there anything I could help with to get a full release?

@ctron
Copy link
Collaborator Author

ctron commented Feb 23, 2024

Is there anything I could help with to get a full release?

Testing, but you done that 😄 I'll also keep testing a bit longer. Maybe I can do a proper release next week.

@extrawurst
Copy link

also curious what we can do to help getting a full release?

@ctron
Copy link
Collaborator Author

ctron commented Feb 29, 2024

also curious what we can do to help getting a full release?

Testing :) There's one more thing I would like to sort out. Which is prefixing (e.g. the wasm) with a sub-directory in the dist folder. That change has landed, but needs some fixing for Windows and some corner cases.

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.

4 participants