fix: Use Build Time Base Path #2964
dpschen
konrad
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
4 Participants
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#2964
Loading…
Reference in New Issue
No description provided.
Delete Branch "eljef/frontend:base_path_fix"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
If a base path is provided at build time, use it.
VIKUNJA_FRONTEND_SUBPATHatbuild time
VIKUNJA_FRONTEND_SUBPATHsetsimport.meta.env.BASE_URLimport.meta.env.BASE_URLare statically replacedat build time.
import.meta.env.BASE_URLdefaults to '/'.
https://vitejs.dev/guide/env-and-mode.html
This uses loadEnv to load an environment file at configuration
time.
VIKUNJA_FRONTEND_SUBPATHis the variable in the environmentfile that will be used to set the base path.
This adds a commented example to .env.local.example
Fixes:
manifest.
wrong path.
in event listener.
incorrect.
/loginvs/base/loginAll of the areas that are patched were identified through running the app in a test deployment using a sub-directory for the base.
This fixes issues with the service worker URL not being correctly set in index.html, the service worker crashing because it can't import workbox correctly, and URLs in the interface redirecting to not include the base URL. (ie, main page sets URL to /login instead of /base/login.
When using
--baseduring the build, the sub-directory must contain a trailing slash.ie:
--base=/base/Hi eljef!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://2964-basepathfix--vikunja-frontend-preview.netlify.app
You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.
Have a nice day!
Hey, thanks for the pull request.
This link is from vue-cli, probably you wantws to paste the other link here as well.
@ -99,3 +99,3 @@routes: [{path: '/',path: import.meta.env.BASE_URL + '',I think vue router has a basepath option (cant't search the docs right now on mobile).
It's part of
createWebHistory: https://router.vuejs.org/api/index.html#createwebhistoryI could not get the basepath options to work, and did not know about the createWebHistory. I can rework this part to use it.
I've changed to use this.
@ -85,3 +85,3 @@srcDir: 'src',filename: 'sw.ts',base: '/',base: process.env.BASE_URL,I'm a bit confused: you mention vites
baseoption but don't use it. This is the base option of VitePWA.Even setting the base option in vite, the PWA setup does not pull that base path. I had to explicitly set it so that the PWA would be pulled from the correct path.
Maybe using
import.meta.env.BASE_URLis the better choice as that will more likely work with the--baseCLI optionprocess.env.BASE_URLis set by --base along withimport.meta.env.BASE_URL.This is why I am using
process.env.BASE_URL.Okay that makes sense.
I am not sure about this. Where in the docs did you get this from? it was the case in vue-cli. In vite you would usually use the
declareoption for statically replacements.fix(base): Use Build Time Baseto fix(base): Use Build Time Base pathThis URL is intended. It actually has the documentation for
process.env.BASE_URL. This page also includes the information for static replacement of variables.This is stated in the documentation here: https://cli.vuejs.org/guide/mode-and-env.html
9a0da60536tobb5f00775fI've pushed an update using
createWebHistorywith a base set as an argument. This does work for me with the subfolder. Thank you @konrad for pointing that out.It's just that we don't use vue cli, only vite. You probably mean https://vitejs.dev/guide/build.html#public-base-path for docs.
Does the process described about this in our docs still work with this change? https://vikunja.io/docs/running-vikunja-in-a-subdirectory/
While vue cli is not used, the variable is still set and used by the underlying vue components.
I followed this process, and ran into the issues that led to me submitting this PR. If you follow the documented process, several areas do not pick up the base path settings. The PWA tries to load from
/instead of/base. As does the manifest, etc. Editing paths in index.html and the manifest leads to the PWA crashing because it is trying to load/workbox-versioninstead of/base/workbox-version.When initiating the first page, the app redirects to
/logininstead of/base/login.Okay, but do we need to update the docs?
If this PR is approved after any requested changes, the only change will be is that
--base=/pathrequires a trailing slash:--base=/path/Can you open a follow up to the docs once this is merged?
I'm 100% willing to do this. What repo are the docs located in?
Awesome! They are here: https://kolaente.dev/vikunja/api/src/branch/main/docs/content/doc/setup/subdirectory.md
PR submitted as WIP until this one is approved.
vikunja/api#1363
bb5f00775ftob98bb50596@ -85,3 +85,3 @@srcDir: 'src',filename: 'sw.ts',base: '/',base: process.env.BASE_URL,Just checking:
If
process.env.BASE_URLis empty akaundefinedwouldn't this set thebaseproperty toundefinedas well? Does VitePWA handle this case, so that it falls back to/?This is the only usage of
process.env.BASE_URLin the patch. Whenbase:is undefined, it falls back to '/'. I've built and tested this a few times now.However, to handle this case as a just in case, I could add a ternary?
process.env.BASE_URL ? process.env.BASE_URL : '/'That will at least set it to '/' in the case of it being falsey (empty / undefined).
./fixes the issue with loading of the PWA from a sub-directory. Using that here.I found this comment from the vue router author. Considering this we really should use vites base option in the configuration and not the cli flag. because else the PWA base might be different.
This turned out to be a lot simpler than I initially thought. Since I can't access
import.meta.env.BASE_URLin vite.config.js, I initially usedprocess.env.BASE_URL. After reading the comment about what the writer said, I did some testing. In just vite.config.js, setting base to './', with the preceding dot, fixes the issue of the service worker not loading correctly. I'll have the updated patch up soon.b98bb50596to783ceb26c4Updated commit in place and it is quite minimal now.
I will explore making this more dynamic in the future. I'd like to not build each release if I can help it :)
I think there's no good way to do this other than building it once with a base url like
BASE_URL_TO_REPLACEand then using sed to replace all instances of that@ -85,3 +85,3 @@srcDir: 'src',filename: 'sw.ts',base: '/',base: './',Sorry to bother, but I'm still not happy with this, because this option of VitePWA needs to be identical to the Vite base option. I am not sure about using ./ because this might create problems when the first page the user visits is a vikunja subpage (like
https://example.com/myvikunjasubpath/labels). The VitePWA base option falls back by default to the Vite base option. So we only need to the Vite base option corectly:(see: https://vite-pwa-org.netlify.app/guide/pwa-minimal-requirements.html#web-app-manifest)
Considering that have you tried removing the base option from VitePWA completely?
So I would do the following:
A ) Set the base option of Vite to
process.env.VIKUNJA_FRONTEND_SUBPATH. I came up with that name by checking our other build options (@konrad what do you think?). A fallback is not necessary (I checked the source). You couldn't useimport.meta.env.BASE_URLbecause that is something that Vite provides to the application after it initialized. Vite itself doesn't have access to it, because it first has to read out the config file :) .B) We than add an example configuration to the
.env.local.examplefile.This way the user can use the normal build instructions using
pnpm run buildand doesn't have to fiddle with vite or workbox. The latter might change either way soonish since we don't really use the up-to-date way to integrate VitePWA.C) This should be documented in vikunja/api#1363
The user can either set the env via the
.envfile or via setting the env dynamically, like:I'm not sure if it would make sense to use something like cross-env there.
Just that I don't loose this link:
I found this as an example in the vite-plugin-pwa repo where they set the env path similar to how I would let the user do it. Not how I would write our commands in the package.json.
I don't think that's necessary since the option is not replaced at runtime, build only. It's probably easier to use the base path option provided by vite?
What impcat does that have to the naming convention?
But the env would be used by vite to set the base path option of vite. If we don't provide the option to set the base path via a env variable we don't conform the 12 factor methodology. It's a common pattern.
Since the base path falls back to the cli flag, it wouldn't prevent the user from using the cli flag. I still wouldn't do it since it makes it more complex since the user needs to use more tools (vite instead of only pnpm and copying the workbox as well).
Not much, I just thought since the other variables using that pattern are used for build-time only this would be confusing. The bigger argument here would be the
--basecli flag which I think will only populate theBASE_URLenv variable.Anyway, that's probably nitpicking now.
I'd argue this does not fall under the 12 factor principle because this is not about configuration at runtime.
It really feels like it, I know! Sorry also @eljef.
I just want to solve this kind configuration also for the future.
As far as I understood storing the config in the environment can also apply to the environment of the build pipeline. This is also exactly what they talk about in that segment.
VIKUNJA_FRONTEND_SUBPATHwould then be build time configuration.I know that we try to make most of the stuff configurable via edits in the index.html which is kind of runtime, but I wanted change this in a way so that we still support this and we can configure the same stuff via environment variables during build time.
This way a user can configure everything in one place if he wants to do his custom build. There wouldn't be the need to change the index.html and use custom vite config options during the build.
I'm used to arguing over patches via mailing lists. The level of nitpicking comes down to quote placement, type of quotes, etc... My feelings are hurt in now way :)
I'd much rather work through all concerns and get it done right the first time.
I'll have the patch edited in a bit. I'll use the environment variable
VIKUNJA_FRONTEND_SUBPATHand will do a bit of correctness checking on it, such as trailing slash check, etc. It's user input, so it's best to sanity check it. I can push that as a config option and use it in the places thatimport.meta.env.BASE_URLis used.This will allow for build pipelines to store the variable for testing as well as keeping the path the same everywhere.
Oh that's nice. I read about using zod for env var checking recently.
I think we only need to set Vites
baseoption withVIKUNJA_FRONTEND_SUBPATH. On all the other places it makes sense to keep usingimport.meta.env.BASE_URL, like you already do, since that's a common pattern.Does this cover the case where the user uses
--baseANDVIKUNJA_FRONTEND_SUBPATH? I don't want to have the--baseoption overrideVIKUNJA_FRONTEND_SUBPATH.If using
VIKUNJA_FRONTEND_SUBPATHoverrides the--baseoption, then I agree.I know that having both shouldn't be something that happens, but I am not above making sure someone is building correctly.
If the user sets the
--baseflag it will override the config that was set in the config file (see here and then used here). But that is fine since we would internally only useimport.meta.env.BASE_URLwhich is based on the resolved config.If there is no env or flag set it will fall back to
/.The
resolveBaseUrlalso does some testing so you don't have to go too far with the tests. I'm actually unsure if you should add any tests, since there might be other cases where a missing/in the end might be wanted/needed by the user.783ceb26c4to020d3e6ec1020d3e6ec1to385e2b389bPatch updated and pushed. The PR's message has been updated.
The base path is now set from
VIKUNJA_FRONTEND_SUBPATH. This correctly sets all paths. This is also future facing as when you implement loading from.envfiles, The resolved environment can set this.I updated the documentation PR to match. I tested builds with and without the variable set. The build correctly sets the paths in both cases.
You can also change the variable name later after a nitpicking match. 😆
Thanks a lot! :)
The only thing I'm missing now is a line in that newly merged .env.local.example filling out our new variable. Can you rebase on top of main?
385e2b389bto1a8fc57ec2Rebased.
Also, to access
.envfiles at configuration time, you need to do a little extra work, per the documentation.https://vitejs.dev/config/#environment-variables
Wow, you are right. I guess the
basedoesn't work right now if, if it is defined via the env file. Only via a var already set via the environment.Can you make it work?
I think I can. Now the question is, should that be included in the scope of this PR?
I think, if it is possible, that I'd like to put the env expansion work in a new PR.
OK, I did not want to close this. Tabbing does not take focus to the UI elements one would think it does :(
Fine by me to do this in another PR 👍
I've updated the documentation PR to not be WIP. I'm working on the loading of the environment file and setting the base via that method.
Let's just change those two lines and then this PR should be ready to go.
Also fine be me!
1a8fc57ec2to3bbbcf3353fix(base): Use Build Time Base pathto Use Build Time Base pathOK. I think I landed on a full, final approach to supporting
VIKUNJA_FRONTEND_SUBPATHand having it properly set throughout the code base.There are two ways of setting the base path supported.
VIKUNJA_FRONTEND_SUBPATH=/subpath/ pnpm vite buildVIKUNJA_FRONTEND_SUBPATHin the env file.I even added a quick check function to vite.config.ts to resolve which build path variable has been specified. It resolves in order of CLI provided environment variable, then .env file, and finally falling back to '/'. The function also verifies the provided path ends with a trailing slash.
I tested with the following conditions:
pnp run buildwith no vars set. (base condition)VIKUNJA_FRONTEND_SUBPATH=/subpath pnpm run buildVIKUNJA_FRONTEND_SUBPATH=/subpath/ pnpm run buildpnpm run buildwithVIKUNJA_FRONTEND_SUBPATH=/subpathset in.env.localpnpm run buildwithVIKUNJA_FRONTEND_SUBPATH=/subpath/set in.env.localpnpm run buildwithVIKUNJA_FRONTEND_SUBPATH=/subpathset in.env.production.localpnpm run buildwithVIKUNJA_FRONTEND_SUBPATH=/subpath/set in.env.production.localAll builds resulted in a properly set base path set in
index.html,manifest.webmanifest, andsw.js. I'll update the documenation PR to reflect.In the future, if you want to set variables to be pulled through the code base during configuration, you can add the define key in configuration and reference env. So, this is future looking by accident ;p
3bbbcf3353toc4f8fd962aLooks good!
@dpschen can you take another look?
I'm really unsure about the requirement of the trailing
/.I vaguely remember having built something on a server where there was never one.
Would this also work for this case?
I do know that vue-router doesn't care if there is a trailing
/or not (see strict option).After checking out the source and history of Vite's config parser I'm also sure that this is not an easy topic, considering the changes that happened there regarding the
baseoption.I found this line in the Vite's source where it gets clear that the base in the resolved config gets added a trailing slash automatically
Might also be that I am now simply confused 😆 after going through all that stuff.
The
loadEnvfunction includes already allprocess.envwhen called with an empty string. So if not a problem otherwise (see paragraph above) we only need to add the trailing slash if the returned config fromloadEnvdoes contain something.If it were only the router, the trailing slash would not be needed. The router does indeed handle the case just fine.
The cases that aren't handled are in the appending of the base path in the service worker. If there is not a trailing slash, your could combine
/basepathand/sw.jsjust fine. But, in the case where base is/, you would end up with//sw.js. I ran into that in testing. And if you register justsw.jswith no path, and then append a base with no trailing slash, you end up with/basesw.js. So, to fix it for this, and for any future URL writes needed outside of the router, I made the function that adds the trailing slash.And in the sw.ts itself, the imports of workbox are similarly affected.
Ugh Gitea.... The tab order for UI elements REALLY needs to be reordered. And why is the close button located so precariously close to the comment button anyway?
c4f8fd962atoe5a4548071e5a4548071to854b883c7cAfter running all sorts of tests, this is the iteration I came up with. The function is removed and instead a basic
ifcheck is used to ensure the base has a trailing slash.The router itself does not need the trailing slash because it has logic to correctly handle the slashes on the base when joining a child path to it.
It is outside the router where things gets hairy. An example is
registerServiceWorker.tsIn order to register the service worker, the base path must be appended in the
register()call. So the path is concatenated. But there are conditions in which the concatenation could go wrong.register(base + '/sw.js')/register(//sw.js)register(base + 'sw.js')/register(/basesw.js)The user doesn't have to provide a trailing slash. I can knock that out of the documentation. The code will handle that for them. But, outside of the router where path resolution and normalization is not happening, the trailing slash is needed to make sure paths are proper.
@dpschen Any news on this after the last push and my last comment?
854b883c7ctoda7fd100e9da7fd100e9to182b5741afPatches for the PR updated to not require any kind of trailing slash.
Places in the code, outside of the router, that use URLs use the urlJoin function to make sure a proper path is built from
import.meta.env.BASE_URLand whatever path is being appended. This will make sure a path doesn't end up being formatted as//pathor/pathsubpathorpath//subpath. There will always be a properpath/subpathor/path/subpath.So far, the only places outside of the router that require this are in
src/registerServiceWorker.tsandsrc/sw.ts.This 100% eliminates any need for a trailing slash and even allows for correction of some user error. ie: If a user were to add slashes after or something.
This does add a dependency on the
url-joinpackage, found at https://github.com/jfromaniello/url-join.I tried the
URLfunction that is supported in browsers, butURLdoes not maintain the/subpathof a URL. It replaces the path. More onURLcan be found here. https://developer.mozilla.org/en-US/docs/Web/API/URL/URLSorry for the delay. I like the last aproach!
Are you sure a client side library like join-url is even needed? Maybe a simple conditional for the leading backslash like you had in the version before would suffice for sw.js and registerserviceworker. Vite has e.g. an internal
joinUrlSegments.I'm taking the blame here because of my bad variable name
VIKUNJA_FRONTEND_SUBPATHI proposed. My idea was to explain better what the variable is. Seems like Vitesbasedid already a good job there. What I mean is: there should never be a path before the base aka subpath, because Vite needs the full path (in the sense ofURL.pathname).If we still nee a lib: We have a url library called ufo already included in Vikunja (see e.g. the use of
parseUrlhere). Maybe we can use thejoinUrlfrom there.The user error correction is something that in this case I think we don't need on client side. If we would check it would probably be better to let the build fail instead of correcting on runtime. But I don't think we should intervene there (not sure right now is there could be a weird valid reason to use
//, e.g. someone assigned your project to that path but you don't have influence on it).The client side library is not needed if we can do the simple check at build time. The simple check makes it so there is no trailing slash needed to be added by the user. It'll be verified at build time and the trailing slash will be added just for the external libraries. I'll gladly revert it back to the simple conditional. The client library was only added because it seemed that you were hoping to eliminate the conditional and backslash all together.
I'll add the conditional and remove the dependency and get this in order.
Awesome! :)
Just to make sure we're on the same page:
I would add the conditional only where it's needed aka where you used
urlJoinin thesw.tsand in theregisterServiceWorker.ts.182b5741afto274a0465e0OK. Updated as requested. Conditionals in
registerServiceWorker.tsandsw.ts. No extra deps. Still honors environment files, etc.Use Build Time Base pathto fix: Use Build Time Base Path274a0465e0to8b212a35438b212a3543to93ed6b9bd393ed6b9bd3tob8859d1559b8859d1559to44c3a0fb4c@dpschen Anything else needed here?
Associated docs updated in vikunja/api#1363
44c3a0fb4ctof6bd88b0def6bd88b0detoaf55992057Incorporated a few changes from @dpschen and rebased, should be good to go now.
Thanks!
Thanks @eljef, sorry again that it took so long!