Skip to content

refactor!: use origin instead of base_url in configuration #979

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

Open
wants to merge 1 commit into
base: 12-23-feat_change_mutation_detection_and_allow_reactive_boolean_defaults
Choose a base branch
from

Conversation

iisakkirotko
Copy link
Collaborator

All Submissions:

Changes to / New Features:

  • I included docs for (the changes to) my feature.

Description of changes

This streamlines the url configuration for Solara server, since we no longer have redundant information in base_url, and both remaining parameters are without trailing slashes.

Copy link
Collaborator Author

iisakkirotko commented Jan 20, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@iisakkirotko iisakkirotko added this to the Solara 2.0 milestone Jan 21, 2025
@iisakkirotko iisakkirotko force-pushed the 12-23-feat_change_mutation_detection_and_allow_reactive_boolean_defaults branch from e925b44 to 70e86a3 Compare January 21, 2025 10:49
@iisakkirotko iisakkirotko force-pushed the 01-20-refactor_use_origin_instead_of_base_url_in_configuration branch 4 times, most recently from b63562e to 4927662 Compare January 21, 2025 14:52
@iisakkirotko iisakkirotko marked this pull request as ready for review January 21, 2025 15:47
@iisakkirotko iisakkirotko force-pushed the 12-23-feat_change_mutation_detection_and_allow_reactive_boolean_defaults branch 3 times, most recently from a97b75b to 1d29010 Compare February 5, 2025 09:06
Streamlines configuration. Whereas previously we had `base_url=origin+root_path`, which forced us to check if the two matched, we now have only the two constituent pieces `origin` and `root_path`
@iisakkirotko iisakkirotko force-pushed the 12-23-feat_change_mutation_detection_and_allow_reactive_boolean_defaults branch from 0cef192 to 2bd9071 Compare February 7, 2025 15:19
@iisakkirotko iisakkirotko force-pushed the 01-20-refactor_use_origin_instead_of_base_url_in_configuration branch from 4927662 to 8687bfb Compare February 7, 2025 15:21
Copy link
Contributor

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

nice work, just the None to signal no configuration would be nice to keep in

Comment on lines +259 to +260
"SOLARA_BASE_URL is deprecated, please use SOLARA_ORIGIN and SOLARA_ROOT_PATH instead. "
"SOLARA_BASE_URL will be removed in a future major version of Solara.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@@ -178,8 +179,8 @@ class MainSettings(BaseSettings):
mode: str = "production"
tracer: bool = False
timing: bool = False
root_path: Optional[str] = None # e.g. /myapp (without trailing slash)
base_url: str = "" # e.g. https://myapp.solara.run/myapp/
root_path: str = "" # e.g. /myapp (without trailing slash)
Copy link
Contributor

Choose a reason for hiding this comment

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

We used the None option to signal it's not configured, which then in starlette.py will recognize it (put a comment there where we do that)

scope = request.scope
root_path_asgi = scope.get("route_root_path", scope.get("root_path", ""))
if settings.main.root_path is None:
if settings.main.root_path == "":
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to keep this to None, to make sure we only do this when it is not explicitly configured.

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