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 support for base url #1873

Merged
merged 2 commits into from
Jun 9, 2024

Conversation

meteyou
Copy link
Member

@meteyou meteyou commented May 3, 2024

Description

This PR allows to use a base url, when building Mainsail. This PR cannot fix a dynamic subdirectory URL. So you have to build your own Mainsail build for each subdirectory.

Maybe it is possible to release a second build in the release for /mainsail as subdirectory, but we have to double-check this, to have no issues with the Moonraker update manager.

Related Tickets & Documents

fixes #1600
fixes a part of #1163

Mobile & Desktop Screenshots/Recordings

none

[optional] Are there any post-deployment tasks we need to perform?

none

@meteyou meteyou marked this pull request as ready for review May 20, 2024 19:27
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label May 20, 2024
@meteyou meteyou requested review from dw-0 and rackrick May 20, 2024 19:27
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 8, 2024
@meteyou meteyou merged commit 113d079 into mainsail-crew:develop Jun 9, 2024
13 checks passed
@meteyou meteyou deleted the feat/add-vite-base-support branch June 9, 2024 17:51
@EricZimmerman
Copy link

how does one actually use this?

@Wulfsta
Copy link

Wulfsta commented Jul 14, 2024

how does one actually use this?

To my understanding, you need to set an environment variable, BASE_URL, at build time, where the value of BASE_URL is the full path of the directory you would like to serve traffic at formatted with a slash as the first character and no trailing slash. @meteyou please confirm.

@EricZimmerman
Copy link

EricZimmerman commented Jul 14, 2024

Meh. I ended up using Caddy and just proxying

Printer.site.com

To http://ip:80 using reverse proxy

Works fine and I don't have to do silly things with compiling a custom version.

If this is done right it should be a property read from a config, not some undocumented build process

@Wulfsta
Copy link

Wulfsta commented Jul 14, 2024

Actually, seems like I'm wrong - this is a built-in environment variable to vite, and should be specified by passing the --base parameter to the build script.

@meteyou
Copy link
Member Author

meteyou commented Jul 14, 2024

@Wulfsta yes you are right. You have to set --base to build the mainsail. Vite is the build tool.

@meteyou
Copy link
Member Author

meteyou commented Jul 14, 2024

@EricZimmerman pls read the issue for this PR: #1600

Pls don't mixup things!

@Wulfsta
Copy link

Wulfsta commented Jul 14, 2024

@meteyou can you clarify the relation between this setting and VUE_APP_PATH? I just spent a few hours finally setting all of this up and seem to have been successful in getting it to work, but am left wondering if my work is correct. I do seem to be seeing a handful of 404s, specifically at {base_url}config.json when first loading the page (my base_url is /voron, so this is trying to load /voronconfig.json), as well as at /img/sidebar-background.svg (which seems to be hardcoded for some reason).

Once you release a new tagged version (and I have time) I will update the configuration in NixOS to add the base option, since it mostly seems to be working.

@meteyou
Copy link
Member Author

meteyou commented Jul 14, 2024

VUE_APP_PATH is only for development when you use vite serve.

If you need help, pls use our discord or the discussions forum.

@Wulfsta
Copy link

Wulfsta commented Jul 14, 2024

I would prefer documentation of this feature to needing to ask on a different platform.

@meteyou
Copy link
Member Author

meteyou commented Jul 14, 2024

@Wulfsta it is not a real "feature". its more like a vue/vite background setting and just fix it, that it works. thats why it is not documented.

if it would be a "generic user feature", then it would need docs. but we are too less people and have too less time to write all the docs. sry...

@Wulfsta
Copy link

Wulfsta commented Jul 14, 2024

Ah, then I should not rely on that environment variable for a downstream build! I produced a build such that it is possible to reverse proxy as requested in the linked issue, but it relies on setting that during build time.

@rackrick
Copy link
Member

rackrick commented Jul 14, 2024

https://cli.vuejs.org/config/#baseurl

There you go, the official Vue documentation.

We just added support for that, therefore it's not really a mainsail feature we just added this because of the feature requests we got.

@meteyou
Copy link
Member Author

meteyou commented Jul 14, 2024

yes, it's really only there to make a build with subdirectories. my plan would be to release 2 builds at some point. one with "/" as base_url and one with "/mainsail". but this also requires some changes in moonraker for updates etc.

@EricZimmerman
Copy link

That seems way easier than just allowing base URL to be supplied in a configuration file like other properties. 😄

Why not 1000 builds so people have options vs / and /mainsail?

@meteyou
Copy link
Member Author

meteyou commented Jul 14, 2024

@EricZimmerman pls tell me a solution to config something in a config file, which cannot be read without this setting?

do you know the chicken, egg problem?

@EricZimmerman
Copy link

I do know it.

It's not personal. Just frustrating when many other apps do this out of the box.

I have my solution and it works well. Caddy made this way easier than nginx

@meteyou
Copy link
Member Author

meteyou commented Jul 14, 2024

@EricZimmerman other apps other dependencies. its just not possible with vue/vite + the websocket communication we use with klipper.

but maybe i'm just to dumb to implement this. so if you think you are much smarter than me, feel free to fork mainsail and solve this issue and merge it. your FR is still open so if some other contributer is smarter than me, can also hit this issue.

otherwise, pls stop posting something like that above and steel my time more and more!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When moonraker is built with --base, it still tries to get config.json from /
4 participants