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: add configurable base URL #1359

Closed

Conversation

ghost
Copy link

@ghost ghost commented Apr 29, 2023

This PR will fix #1163

Adds a VITE_BASE environment variable at build-time that allows the user to serve the app from non-root URL paths.

Signed-off-by: Alice [email protected]

@ghost ghost marked this pull request as draft April 29, 2023 19:12
@ghost ghost force-pushed the configurable-base branch from 0dd82a3 to fe2a11d Compare April 29, 2023 19:22
@ghost ghost marked this pull request as ready for review April 29, 2023 19:22
@ghost
Copy link
Author

ghost commented Apr 29, 2023

this is now working on my machine, so it should be good to review. the only thing not checked is the github actions changes (also, it's up to the maintainers whether you want to have a second build or not, it would impact your project's storage usage slightly)

@ghost ghost force-pushed the configurable-base branch from 0d8bb72 to 432605f Compare April 29, 2023 21:22
@ghost
Copy link
Author

ghost commented Apr 29, 2023

note: this addresses all the issues in my own setup, but there's probably other paths I've missed. I'm not a web developer so this was all pulled together through trial and error, there should probably be a test case added to catch any regressions / missed cases

@meteyou
Copy link
Member

meteyou commented Apr 30, 2023

Thx for your PR. This PR with the changed build workflow would "kill" all default installations. So the default should not be changed.

I have to check the rest on my test rig.

@meteyou
Copy link
Member

meteyou commented Apr 30, 2023

I would prefer a dynamic solution (editable via config.json file). I see a separate build for subdirectories as a workaround. I couldn't test the build yet, but I wouldn't be sure now if the API would also run via reverse proxy.

@ghost
Copy link
Author

ghost commented Apr 30, 2023

that would be ideal, unfortunately a lot of the static assets are compiled at build time that contain absolute paths, it's possible some of them could be changed to relative paths but with the virtual multi-page system I'm not sure how that would interact. I don't love this solution either but I didn't see any other approach to take that wouldn't involve major architectural changes.

If you don't want to publish multiple builds, I can remove the github actions changes, I think just upstreaming this build option would already be a huge win for weird nerds like myself who want to serve each webapp at its own subpath. I'd be happy to write some docs for this feature as well, let me know where to look and if that's in-repo I can put it in this PR.

As for moonraker, this PR shouldn't change anything about where it looks for moonraker (/moonraker), I'd like to make that configurable as well but that's outside of the scope of this PR.

@meteyou
Copy link
Member

meteyou commented May 1, 2023

Are you able to use the Moonraker update manager to use the subpath?

@ghost
Copy link
Author

ghost commented May 1, 2023

i don't think so, looks like that would require adding functionality to Moonraker to set build-time environment variables

@meteyou
Copy link
Member

meteyou commented May 1, 2023

nono... moonraker is not building mainsail. it just download the prebuild zip from the latest release.

@ghost
Copy link
Author

ghost commented May 1, 2023

oh right, my bad. taking a look at the update process, looks like right now it's hardcoded to just use the first asset associated with a Release:
https://github.com/Arksine/moonraker/blob/fd5ea0c6a45fb04baf21014d1505f3d4ade791db/moonraker/components/update_manager/update_manager.py#L1277-L1278

So unless that's updated to iterate through the release assets with some name matching logic exposed as a config option, I should remove the github actions part from this PR.

I'm busy with work for the next while but I can try to follow up on this later this week, either creating a moonraker PR to add that logic to its updater or just removing the release part of this PR if you'd rather defer that for now

@ghost ghost force-pushed the configurable-base branch 2 times, most recently from 68111c4 to 35bddc5 Compare May 22, 2023 20:39
Alice added 12 commits May 22, 2023 15:41
this is a base env file, it's ok to include this in the repo. The one that should be excluded is .env.local, and that's already covered by the *.local line

Signed-off-by: Alice <[email protected]>
Signed-off-by: Alice <[email protected]>
@ghost ghost force-pushed the configurable-base branch from 35bddc5 to bd5f43c Compare May 22, 2023 20:53
@ghost
Copy link
Author

ghost commented May 22, 2023

Sorry for the delay. I've reverted the commit that adds the github builds, that should prevent any issues with Moonraker's auto-updater accidentally grabbing the wrong build. I've also rebased to clear up some merge commits, and it looks like #1336 made a lot of this PR's changes redundant, so the diff should look a lot smaller now

@ghost ghost marked this pull request as draft May 22, 2023 20:56
@ghost
Copy link
Author

ghost commented May 22, 2023

looks like I introduced an error, I'll un-draft this once that's fixed

@ghost ghost force-pushed the configurable-base branch from cbf41dc to dfefe8e Compare May 23, 2023 00:01
@ghost ghost force-pushed the configurable-base branch from dfefe8e to 63adcdd Compare May 23, 2023 00:02
@meteyou
Copy link
Member

meteyou commented Oct 1, 2023

i will close this PR, because of no response... Please open a new PR, if you have an idea how to fix this issue.

@meteyou meteyou closed this Oct 1, 2023
@ghost
Copy link
Author

ghost commented Oct 2, 2023

totally understandable, I haven't had time to work on this, and it's sat too long by now

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.

Add nginx reverse proxy support
1 participant