-
Notifications
You must be signed in to change notification settings - Fork 95
feat: add base path to front end #1393
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,24 @@ | ||
| import { getAuthToken } from "src/lib/auth"; | ||
| import { ErrorResponse } from "src/types"; | ||
|
|
||
| const BASE_URL = import.meta.env.BASE_URL; | ||
| const PREFIXES = ["/api", "/images"]; | ||
|
|
||
| function startsWithPrefix(path: string, prefixes: string[]): boolean { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the changes in this file should be needed - requests always go to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just do:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Running I only added it to the
The const About the path concatenation, yes I think so, but it would need to be some kind of Best is probably to add a small joinPath function.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I simplified this in 8de2fdb0#1 and it works from my testing with caddy. Please let me know if that works for you. One thing to note is you will need to update to use |
||
| return prefixes.some((prefix) => path.startsWith(prefix)); | ||
| } | ||
|
|
||
| export const request = async <T>( | ||
| ...args: Parameters<typeof fetch> | ||
| ): Promise<T | undefined> => { | ||
| if ( | ||
| BASE_URL !== "/" && | ||
| typeof args[0] === "string" && | ||
| startsWithPrefix(args[0], PREFIXES) | ||
| ) { | ||
| args[0] = BASE_URL + args[0].slice(1); | ||
| } | ||
|
|
||
| const token = getAuthToken(); | ||
| if (token) { | ||
| if (!args[1]) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is
BASE_PATHandBASE_URL, we only need one, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently
BASE_URLis an already declared property used by the hub go app.Also note there is a
FRONTEND_URLused by the go app too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
BASE_URLis a vite specific, I think they call it URL since they allow to set the whole URL in cases resources come from a specific server (CDN for example). This feature is focused on allowing to server the frontend behind a proxy using sub path. That case is usually namedbase_pathand I wanted to keep the top level variable reflecting that specific use case. URL would suggest to allow to change the whole URL.I could't see a specific group set up for the
/apipath in the echo framework boilerplate, and a quick search seems to suggestFRONTEND_URLandBASE_PATHis used when connecting the alby account with the local hub instance ( from defaulthttps://getalby.com/).Yes that has a potential for creating confusion. Maybe the
BASE_PATHshould be calledVITE_FRONTEND_BASE_PATH.Since you have a better understanding of the already existing variables, do you have any suggestion that helps differentiate ?
And I would really not focus on
BASE_URLsince it is a vite internal and is a constant injected into the build artefact. Runtime env variables won't reflect on that, it is just build time relevant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the details, after looking into it I understand this is how vite works. And the FRONTEND_URL we have is somewhat different and only applies to Alby Cloud. So that one can probably be ignored for now.
I made some changes in this PR - could you review?
8de2fdb0#1