fix: Use Build Time Base Path #2964

Merged
konrad merged 2 commits from eljef/frontend:base_path_fix into main 2023-02-03 08:57:29 +00:00
6 changed files with 156 additions and 134 deletions
Showing only changes of commit e92559dc00 - Show all commits

View File

@ -0,0 +1,14 @@
/**
* Get full BASE_URL
* - including path
* - will always end with a trailing slash
*/
export function getFullBaseUrl() {
// (1) The injected BASE_URL is declared from the `resolvedBase` that might miss a trailing slash...
// see: https://github.com/vitejs/vite/blob/b35fe883fdc699ac1450882562872095abe9959b/packages/vite/src/node/config.ts#LL614C25-L614C25
const rawBase = import.meta.env.BASE_URL
// (2) so we readd a slash like done here
// https://github.com/vitejs/vite/blob/b35fe883fdc699ac1450882562872095abe9959b/packages/vite/src/node/config.ts#L643
// See this comment: https://github.com/vitejs/vite/pull/10723#issuecomment-1303627478
return rawBase.endsWith('/') ? rawBase : rawBase + '/'
}

View File

@ -2,8 +2,10 @@
import {register} from 'register-service-worker'
import {getFullBaseUrl} from './helpers/getFullBaseUrl'
if (import.meta.env.PROD) {
register('/sw.js', {
register(getFullBaseUrl() + 'sw.js', {
ready() {
console.log('App is being served from cache by a service worker.')
},

View File

@ -81,7 +81,7 @@ const EditTeamComponent = () => import('@/views/teams/EditTeam.vue')
const NewTeamComponent = () => import('@/views/teams/NewTeam.vue')
const router = createRouter({
history: createWebHistory(),
history: createWebHistory(import.meta.env.BASE_URL),
scrollBehavior(to, from, savedPosition) {
// If the user is using their forward/backward keys to navigate, we want to restore the scroll view
if (savedPosition) {

View File

@ -1,10 +1,16 @@
/* eslint-disable no-console */
/* eslint-disable no-undef */
import {getFullBaseUrl} from './helpers/getFullBaseUrl'
declare let self: ServiceWorkerGlobalScope
const fullBaseUrl = getFullBaseUrl()
const workboxVersion = 'v6.5.4'
importScripts( `/workbox-${workboxVersion}/workbox-sw.js`)
importScripts(`${fullBaseUrl}workbox-${workboxVersion}/workbox-sw.js`)
workbox.setConfig({
modulePathPrefix: `/workbox-${workboxVersion}`,
modulePathPrefix: `${fullBaseUrl}workbox-${workboxVersion}`,
debug: Boolean(import.meta.env.VITE_WORKBOX_DEBUG),
})
@ -47,7 +53,7 @@ self.addEventListener('notificationclick', function (event) {
switch (event.action) {
case 'show-task':
clients.openWindow(`/tasks/${taskId}`)
clients.openWindow(`${fullBaseUrl}tasks/${taskId}`)
break
}
})

View File

@ -5,7 +5,7 @@
"compilerOptions": {
"composite": true,
"baseUrl": ".",
"lib": ["ESNext"],
"lib": ["ESNext", "DOM", "WebWorker"],
"importHelpers": true,
"sourceMap": true,

View File

@ -2,13 +2,13 @@
import {defineConfig, type PluginOption} from 'vite'
import vue from '@vitejs/plugin-vue'
import legacyFn from '@vitejs/plugin-legacy'
import { URL, fileURLToPath } from 'node:url'
import { dirname, resolve } from 'node:path'
import {URL, fileURLToPath} from 'node:url'
import {dirname, resolve} from 'node:path'
import VueI18nPlugin from '@intlify/unplugin-vue-i18n/vite'
import {VitePWA} from 'vite-plugin-pwa'
import {VitePWA} from 'vite-plugin-pwa'
import VitePluginInjectPreload from 'vite-plugin-inject-preload'
import {visualizer} from 'rollup-plugin-visualizer'
import {visualizer} from 'rollup-plugin-visualizer'
import svgLoader from 'vite-svg-loader'
import postcssPresetEnv from 'postcss-preset-env'
import postcssEasings from 'postcss-easings'
@ -50,135 +50,135 @@ function createFontMatcher(fontNames: string[]) {
// https://vitejs.dev/config/
export default defineConfig({
// https://vitest.dev/config/
test: {
environment: 'happy-dom',
},
css: {
preprocessorOptions: {
scss: {
additionalData: PREFIXED_SCSS_STYLES,
charset: false, // fixes "@charset" must be the first rule in the file" warnings
},
base: process.env.VIKUNJA_FRONTEND_BASE,
// https://vitest.dev/config/
test: {
environment: 'happy-dom',
},
css: {
preprocessorOptions: {
scss: {
additionalData: PREFIXED_SCSS_STYLES,
charset: false, // fixes "@charset" must be the first rule in the file" warnings
},
postcss: {
plugins: [
postcssEasings(),
postcssEasingGradients(),
},
postcss: {
plugins: [
postcssEasings(),
postcssEasingGradients(),
postcssPresetEnv(),
],
},
},
plugins: [
vue({
reactivityTransform: true,
}),
legacy,
svgLoader({
// Since the svgs are already manually optimized via https://jakearchibald.github.io/svgomg/
// we don't need to optimize them again.
svgo: false,
}),
VueI18nPlugin({
// TODO: only install needed stuff
// Whether to install the full set of APIs, components, etc. provided by Vue I18n.
// By default, all of them will be installed.
fullInstall: true,
include: resolve(dirname(pathSrc), './src/i18n/lang/**'),
}),
// https://github.com/Applelo/vite-plugin-inject-preload
VitePluginInjectPreload({
files: [{
match: createFontMatcher(['Quicksand', 'OpenSans', 'OpenSans-Italic']),
attributes: {crossorigin: 'anonymous'},
}],
injectTo: 'custom',
}),
VitePWA({
srcDir: 'src',
filename: 'sw.ts',
base: '/',
strategies: 'injectManifest',
injectRegister: false,
manifest: {
name: 'Vikunja',
short_name: 'Vikunja',
theme_color: '#1973ff',
icons: [
{
src: './images/icons/android-chrome-192x192.png',
sizes: '192x192',
type: 'image/png',
},
{
src: './images/icons/android-chrome-512x512.png',
sizes: '512x512',
type: 'image/png',
},
{
src: './images/icons/icon-maskable.png',
sizes: '1024x1024',
type: 'image/png',
purpose: 'maskable',
},
],
start_url: '.',
display: 'standalone',
background_color: '#000000',
shortcuts: [
{
name: 'Overview',
url: '/',
},
{
name: 'Namespaces And Lists Overview',
short_name: 'Namespaces & Lists',
url: '/namespaces',
},
{
name: 'Tasks Next Week',
short_name: 'Next Week',
url: '/tasks/by/week',
},
{
name: 'Tasks Next Month',
short_name: 'Next Month',
url: '/tasks/by/month',
},
{
name: 'Teams Overview',
short_name: 'Teams',
url: '/teams',
},
],
},
}),
],
resolve: {
alias: [
{
find: '@',
replacement: pathSrc,
},
],
extensions: ['.mjs', '.js', '.ts', '.jsx', '.tsx', '.json', '.vue'],
},
server: {
host: '127.0.0.1', // see: https://github.com/vitejs/vite/pull/8543
port: 4173,
strictPort: true,
},
build: {
target: 'esnext',
rollupOptions: {
plugins: [
visualizer({
filename: 'stats.html',
gzipSize: true,
// template: 'sunburst',
// brotliSize: true,
}) as PluginOption,
},
plugins: [
vue({
reactivityTransform: true,
}),
legacy,
svgLoader({
// Since the svgs are already manually optimized via https://jakearchibald.github.io/svgomg/
// we don't need to optimize them again.
svgo: false,
}),
VueI18nPlugin({
// TODO: only install needed stuff
// Whether to install the full set of APIs, components, etc. provided by Vue I18n.
// By default, all of them will be installed.
fullInstall: true,
eljef marked this conversation as resolved Outdated

I'm a bit confused: you mention vites base option but don't use it. This is the base option of VitePWA.

I'm a bit confused: you mention vites `base` option but don't use it. This is the base option of VitePWA.
Outdated
Review

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.

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 option

Maybe using `import.meta.env.BASE_URL` is the better choice as that will more likely work with the `--base` CLI option
Outdated
Review

process.env.BASE_URL is set by --base along with import.meta.env.BASE_URL.

WARNING] "import.meta" is not available with the "cjs" output format and will be empty [empty-import-meta]

vite.config.ts:87:9:
87 │ base: import.meta.env.BASE_URL,
╵ ~~~~~~~~~~~

You need to set the output format to "esm" for "import.meta" to work correctly.

This is why I am using process.env.BASE_URL.

`process.env.BASE_URL` is set by --base along with `import.meta.env.BASE_URL`. >WARNING] "import.meta" is not available with the "cjs" output format and will be empty [empty-import-meta] > > vite.config.ts:87:9: > 87 │ base: import.meta.env.BASE_URL, > ╵ ~~~~~~~~~~~ > > You need to set the output format to "esm" for "import.meta" to work correctly. This is why I am using `process.env.BASE_URL`.

Okay that makes sense.

Okay that makes sense.

Just checking:
If process.env.BASE_URL is empty aka undefined wouldn't this set the base property to undefined as well? Does VitePWA handle this case, so that it falls back to /?

Just checking: If `process.env.BASE_URL` is empty aka `undefined` wouldn't this set the `base` property to `undefined` as well? Does VitePWA handle this case, so that it falls back to `/`?
Outdated
Review

This is the only usage of process.env.BASE_URL in the patch. When base: 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).

This is the only usage of `process.env.BASE_URL` in the patch. When `base:` 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).
Outdated
Review

./ fixes the issue with loading of the PWA from a sub-directory. Using that here.

`./` fixes the issue with loading of the PWA from a sub-directory. Using that here.

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:

a scope: omitted here for simplicity, the vite-plugin-pwa plugin will use the Vite base option to configure it (default is /)

(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 use import.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:

VIKUNJA_FRONTEND_SUBPATH=myvikunjasubpath && pnpm run build

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.

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: > a scope: omitted here for simplicity, the vite-plugin-pwa plugin will use the Vite base option to configure it (default is /) *(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](https://vikunja.io/docs/config-options/) (@konrad what do you think?). A fallback is not necessary (I checked the source). You couldn't use `import.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](https://kolaente.dev/vikunja/frontend/src/branch/main/.env.local.example). 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 https://kolaente.dev/vikunja/api/pulls/1363 The user can either set the env via the `.env` file or via setting the env dynamically, like: ```sh VIKUNJA_FRONTEND_SUBPATH=myvikunjasubpath && pnpm run build ``` I'm not sure if it would make sense to use something like [cross-env](https://github.com/kentcdodds/cross-env) there. ------ Just that I don't loose this link: I found [this](https://github.com/vite-pwa/vite-plugin-pwa/blob/3d6f489b360c971b3a376a1284c34aa260fb03aa/examples/vue-router/package.json#L19-L26) 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 came up with that name by checking our other build options (@konrad what do you think?)

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?

> I came up with that name by checking our other build options (@konrad what do you think?) 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?

I don't think that's necessary since the option is not replaced at runtime, build only.

What impcat does that have to the naming convention?

It's probably easier to use the base path option provided by vite?

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).

> I don't think that's necessary since the option is not replaced at runtime, build only. What impcat does that have to the naming convention? > It's probably easier to use the base path option provided by vite? 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](https://sourcegraph.com/search?q=context%3Aglobal+file%3Avite%5C.config+base%3A+process.env.BASE_URL&patternType=standard&sm=1&groupBy=repo). 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).

What impcat does that have to the naming convention?

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 the BASE_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.

> What impcat does that have to the naming convention? 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 the `BASE_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*.

Anyway, that's probably nitpicking now.

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.

since the other variables using that pattern are used for build-time only

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.

> Anyway, that's probably nitpicking now. 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](https://12factor.net/config) can also apply to the environment of the build pipeline. This is also exactly what they talk about in that segment. > since the other variables using that pattern are used for build-time only `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.
Outdated
Review

Anyway, that's probably nitpicking now.

It really feels like it, I know! Sorry also @eljef.

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 that import.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.

> Anyway, that's probably nitpicking now. > > It really feels like it, I know! Sorry also @eljef. 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 that `import.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.

will do a bit of correctness checking on it, such as trailing slash check, etc.

Oh that's nice. I read about using zod for env var checking recently.

I can push that as a config option and use it in the places that import.meta.env.BASE_URL is used.

I think we only need to set Vites base option with VIKUNJA_FRONTEND_SUBPATH. On all the other places it makes sense to keep using import.meta.env.BASE_URL, like you already do, since that's a common pattern.

> will do a bit of correctness checking on it, such as trailing slash check, etc. Oh that's nice. I read about [using zod for env var checking](https://twitter.com/mattpocockuk/status/1615110808219918352) recently. > I can push that as a config option and use it in the places that import.meta.env.BASE_URL is used. I think we only need to set Vites `base` option with `VIKUNJA_FRONTEND_SUBPATH`. On all the other places it makes sense to keep using `import.meta.env.BASE_URL`, like you already do, since that's a common pattern.
Outdated
Review

I think we only need to set Vites base option with VIKUNJA_FRONTEND_SUBPATH.

Does this cover the case where the user uses --base AND
VIKUNJA_FRONTEND_SUBPATH? I don't want to have the --base option override VIKUNJA_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.

> I think we only need to set Vites base option with VIKUNJA_FRONTEND_SUBPATH. Does this cover the case where the user uses `--base` AND `VIKUNJA_FRONTEND_SUBPATH`? I don't want to have the `--base` option override `VIKUNJA_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 use import.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.

If the user sets the `--base` flag it **will** override the config that was set in the config file (see [here](https://github.com/vitejs/vite/blob/a6df6b4a21897e15c4ec0ae1ca4070eaa5d16946/packages/vite/src/node/config.ts#LL1109C23-L1109C23) and then used [here](https://github.com/vitejs/vite/blob/a6df6b4a21897e15c4ec0ae1ca4070eaa5d16946/packages/vite/src/node/config.ts#L442)). But that is fine since we would internally only use `import.meta.env.BASE_URL` which is based on the resolved config. If there is no env or flag set [it will fall back to `/`](https://github.com/vitejs/vite/blob/a6df6b4a21897e15c4ec0ae1ca4070eaa5d16946/packages/vite/src/node/config.ts#L522). 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.
include: resolve(dirname(pathSrc), './src/i18n/lang/**'),
}),
// https://github.com/Applelo/vite-plugin-inject-preload
VitePluginInjectPreload({
files: [{
match: createFontMatcher(['Quicksand', 'OpenSans', 'OpenSans-Italic']),
attributes: {crossorigin: 'anonymous'},
}],
injectTo: 'custom',
}),
VitePWA({
srcDir: 'src',
filename: 'sw.ts',
strategies: 'injectManifest',
injectRegister: false,
manifest: {
name: 'Vikunja',
short_name: 'Vikunja',
theme_color: '#1973ff',
icons: [
{
src: './images/icons/android-chrome-192x192.png',
sizes: '192x192',
type: 'image/png',
},
{
src: './images/icons/android-chrome-512x512.png',
sizes: '512x512',
type: 'image/png',
},
{
src: './images/icons/icon-maskable.png',
sizes: '1024x1024',
type: 'image/png',
purpose: 'maskable',
},
],
start_url: '.',
display: 'standalone',
background_color: '#000000',
shortcuts: [
{
name: 'Overview',
url: '/',
},
{
name: 'Namespaces And Lists Overview',
short_name: 'Namespaces & Lists',
url: '/namespaces',
},
{
name: 'Tasks Next Week',
short_name: 'Next Week',
url: '/tasks/by/week',
},
{
name: 'Tasks Next Month',
short_name: 'Next Month',
url: '/tasks/by/month',
},
{
name: 'Teams Overview',
short_name: 'Teams',
url: '/teams',
},
],
},
}),
],
resolve: {
alias: [
{
find: '@',
replacement: pathSrc,
},
],
extensions: ['.mjs', '.js', '.ts', '.jsx', '.tsx', '.json', '.vue'],
},
server: {
host: '127.0.0.1', // see: https://github.com/vitejs/vite/pull/8543
port: 4173,
strictPort: true,
},
build: {
target: 'esnext',
rollupOptions: {
plugins: [
visualizer({
filename: 'stats.html',
gzipSize: true,
// template: 'sunburst',
// brotliSize: true,
}) as PluginOption,
],
},
},
})