fix: Use Build Time Base Path #2964
Loading…
x
Reference in New Issue
Block a user
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_SUBPATH
atbuild time
VIKUNJA_FRONTEND_SUBPATH
setsimport.meta.env.BASE_URL
import.meta.env.BASE_URL
are statically replacedat build time.
import.meta.env.BASE_URL
defaults to '/'.
https://vitejs.dev/guide/env-and-mode.html
This uses loadEnv to load an environment file at configuration
time.
VIKUNJA_FRONTEND_SUBPATH
is 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.
/login
vs/base/login
All 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
--base
during 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
base
option 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_URL
is the better choice as that will more likely work with the--base
CLI optionprocess.env.BASE_URL
is 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
declare
option 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
9a0da60536
tobb5f00775f
I've pushed an update using
createWebHistory
with 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-version
instead of/base/workbox-version
.When initiating the first page, the app redirects to
/login
instead 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=/path
requires 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
bb5f00775f
tob98bb50596
@ -85,3 +85,3 @@
srcDir: 'src',
filename: 'sw.ts',
base: '/',
base: process.env.BASE_URL,
Just checking:
If
process.env.BASE_URL
is empty akaundefined
wouldn't this set thebase
property toundefined
as well? Does VitePWA handle this case, so that it falls back to/
?This is the only usage of
process.env.BASE_URL
in 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_URL
in 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.b98bb50596
to783ceb26c4
Updated 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_REPLACE
and 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_URL
because 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.example
file.This way the user can use the normal build instructions using
pnpm run build
and 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
.env
file 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
--base
cli flag which I think will only populate theBASE_URL
env 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_SUBPATH
would 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_SUBPATH
and 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_URL
is 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
base
option 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
--base
ANDVIKUNJA_FRONTEND_SUBPATH
? I don't want to have the--base
option overrideVIKUNJA_FRONTEND_SUBPATH
.If using
VIKUNJA_FRONTEND_SUBPATH
overrides the--base
option, 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
--base
flag 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_URL
which is based on the resolved config.If there is no env or flag set it will fall back to
/
.The
resolveBaseUrl
also 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.783ceb26c4
to020d3e6ec1
020d3e6ec1
to385e2b389b
Patch 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.env
files, 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?
385e2b389b
to1a8fc57ec2
Rebased.
Also, to access
.env
files 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
base
doesn'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!
1a8fc57ec2
to3bbbcf3353
fix(base): Use Build Time Base pathto Use Build Time Base pathOK. I think I landed on a full, final approach to supporting
VIKUNJA_FRONTEND_SUBPATH
and having it properly set throughout the code base.There are two ways of setting the base path supported.
VIKUNJA_FRONTEND_SUBPATH=/subpath/ pnpm vite build
VIKUNJA_FRONTEND_SUBPATH
in 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 build
with no vars set. (base condition)VIKUNJA_FRONTEND_SUBPATH=/subpath pnpm run build
VIKUNJA_FRONTEND_SUBPATH=/subpath/ pnpm run build
pnpm run build
withVIKUNJA_FRONTEND_SUBPATH=/subpath
set in.env.local
pnpm run build
withVIKUNJA_FRONTEND_SUBPATH=/subpath/
set in.env.local
pnpm run build
withVIKUNJA_FRONTEND_SUBPATH=/subpath
set in.env.production.local
pnpm run build
withVIKUNJA_FRONTEND_SUBPATH=/subpath/
set in.env.production.local
All 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
3bbbcf3353
toc4f8fd962a
Looks 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
base
option.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
loadEnv
function includes already allprocess.env
when 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 fromloadEnv
does 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
/basepath
and/sw.js
just 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.js
with 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?
c4f8fd962a
toe5a4548071
e5a4548071
to854b883c7c
After running all sorts of tests, this is the iteration I came up with. The function is removed and instead a basic
if
check 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.ts
In 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?
854b883c7c
toda7fd100e9
da7fd100e9
to182b5741af
Patches 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_URL
and whatever path is being appended. This will make sure a path doesn't end up being formatted as//path
or/pathsubpath
orpath//subpath
. There will always be a properpath/subpath
or/path/subpath
.So far, the only places outside of the router that require this are in
src/registerServiceWorker.ts
andsrc/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-join
package, found at https://github.com/jfromaniello/url-join.I tried the
URL
function that is supported in browsers, butURL
does not maintain the/subpath
of a URL. It replaces the path. More onURL
can 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_SUBPATH
I proposed. My idea was to explain better what the variable is. Seems like Vitesbase
did 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
parseUrl
here). Maybe we can use thejoinUrl
from 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
urlJoin
in thesw.ts
and in theregisterServiceWorker.ts
.182b5741af
to274a0465e0
OK. Updated as requested. Conditionals in
registerServiceWorker.ts
andsw.ts
. No extra deps. Still honors environment files, etc.Use Build Time Base pathto fix: Use Build Time Base Path274a0465e0
to8b212a3543
8b212a3543
to93ed6b9bd3
93ed6b9bd3
tob8859d1559
b8859d1559
to44c3a0fb4c
@dpschen Anything else needed here?
Associated docs updated in vikunja/api#1363
44c3a0fb4c
tof6bd88b0de
f6bd88b0de
toaf55992057
Incorporated a few changes from @dpschen and rebased, should be good to go now.
Thanks!
Thanks @eljef, sorry again that it took so long!