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

Allow for customization of path_prefix_server by setting Turbine field. #292

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

giacomocariello
Copy link

Currently only a single Perseus app can be served by a single server backend. While Perseus already allows for customization of app base path, this customization is activated by setting PERSEUS_BASE_PATH environment variable, which means that the same path prefix is applied process-wide, so it's not possible to create a non-standard server implementation "mounting" two Turbines on different URL mountpoints.
This means that the only way to have multiple Perseus apps running under the same origin is to use a reverse proxy to forward requests to different Perseus servers.
This PR is meant to allow setting a path_prefix_server on a Turbine, while leaving the default behavior of resolving PERSEUS_BASE_PATH environment variable if a custom value is not set.

from free function to Turbine public method. This way,
path_prefix_server can be customized in order to build non-standard
combinations of multiple Perseus apps.
@arctic-hen7
Copy link
Member

Hey, sorry for the late reply, I've been really busy lately, but I am back now! I think this is a good idea in theory, but I don't love the idea of moving this into the code. From my perspective, the base path is very much a configuration value, and implanting that into an app's code isn't amenable to the philosophy of keeping configuration that can change easily (e.g. an app should be able to be hosted wherever you like once the code is ready). Also, to clarify, your use-case for this is running two turbines in the same server? Is there any particular reason you can't run the two as separate processes?

@giacomocariello
Copy link
Author

I get your point, however several other "configurations" are currently available for customization in code. I believe this is for good reason: instead of wiring the developer onto a specific way of managing configuration (environment vars), the developer should be free to decide for, say, a configuration file.
That said, my use case is to split the client-side wasm part into sub-apps for different areas of the site, to keep load time low. On the server-side this split would require an unnecessary larger footprint with multiple server processes and some front-end proxying.

@arctic-hen7
Copy link
Member

Okay, I certainly accept that, and your use-case is far less niche than I was expecting! Let me think about this some more and review your code in greater depth.

Copy link
Member

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor wording change in one comment, otherwise this is good to merge!

I've thought about it a bit more and I think, given this is such an important configuration element, it's probably a good idea to increase the flexibility of things by having it be an option that can be specified in the code. Thanks very much for the suggestion and the PR!

packages/perseus/src/turbine/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you so much!

@arctic-hen7
Copy link
Member

@giacomocariello would you mind resolving the conflicts with main? Then this is good to merge.

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