matze / wastebin

wastebin is a pastebin 📝
https://bin.bloerg.net
MIT License
336 stars 27 forks source link

Handle serving from a sub-path #48

Closed aldur closed 8 months ago

aldur commented 9 months ago

Hi there! 👋

I took a shot at letting wastebin work when served from a sub-path (e.g. https://example.com/wastsebin/). It's a WIP, but I thought of getting this out early and discuss 1. if useful; 2. improvements.

The idea is to re-use the WASTEBIN_BASE_URL variable and get its path component (if any).

Right now:

What's the best approach to add the test? Duplicate them? Run them in a for loop with / without the sub-path? Also, I am not convinced of storing the base path in the metadata as a static variable. Are there other approaches?

matze commented 9 months ago

First of all, the feature is certainly welcome, thank you :+1:

Also, I am not convinced of storing the base path in the metadata as a static variable. Are there other approaches?

I also don't think it's the best approach and rather compute it at startup and store it in a OnceCell. Also, it could make sense to abstract the BaseUrl in a Url newtype and avoid the manual joins in the templates and elsewhere.

I will have a closer look and more comments by the end of the week.

aldur commented 9 months ago

I also don't think it's the best approach and rather compute it at startup and store it in a OnceCell. Also, it could make sense to abstract the BaseUrl in a Url newtype and avoid the manual joins in the templates and elsewhere.

https://github.com/matze/wastebin/pull/48/commits/cbb71a1a28c0a0a21af786e25e8705d131aed86f gives a shot at addressing this. I have added a newtype for BasePath and used OnceLock to initialise it.

I am not a super fan of handling all errors there, but raising them would require a bigger change. Happy to hear thoughts about it. Also, we need to add a trailing slash to the base path. I do it if missing, but we might decide to validate/raise instead.

Overall kept things pretty simple, but the code looks cleaner now by using BasePath::join. Maybe BasePath could live somewhere else than env, but that's easy to change.

I considered adding a newtype for the BaseUrl too, but eventually decided against it as it doesn't add much value.

Also, open to comments about the best way to test this. We could duplicate the tests and/or the client, but maybe there are better approaches (e.g. test parametrisation, fixtures, ecc).

matze commented 9 months ago

Also, open to comments about the best way to test this.

Up to you, so far things look good to me. If you think it's ready, let me know.

aldur commented 8 months ago

Done! I have considered using parametrised tests, but since we use static variables things get complicated pretty fast.

In the end I have opted for simplicity: the tests respect BASE_URL and the CI runs both on / and on /wastebin.

Ready from my point of view!

matze commented 8 months ago

Thanks, I will let it simmer for a bit and try to make a new release soon enough.

aldur commented 8 months ago

Thank you! When you do release, could you also update the README? I forgot to do that.