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

Handle serving from a sub-path #48

Merged
merged 5 commits into from
Feb 25, 2024
Merged

Conversation

aldur
Copy link
Contributor

@aldur aldur commented Feb 12, 2024

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:

  • Existing tests pass.
  • It works™.
  • If required, clean-up.
  • Add tests for the sub-path implementation.

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
Copy link
Owner

matze commented Feb 12, 2024

First of all, the feature is certainly welcome, thank you 👍

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

aldur commented Feb 18, 2024

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.

cbb71a1 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
Copy link
Owner

matze commented Feb 23, 2024

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

aldur commented Feb 24, 2024

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 matze merged commit dc61e4a into matze:master Feb 25, 2024
1 check passed
@matze
Copy link
Owner

matze commented Feb 25, 2024

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

@aldur aldur deleted the handle_base_path branch February 26, 2024 09:29
@aldur
Copy link
Contributor Author

aldur commented Feb 26, 2024

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

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