feat: defer everything until the api config is loaded #926
No reviewers
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
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#926
Loading…
Reference in New Issue
No description provided.
Delete Branch "feature/ready-state"
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?
This PR adds a wrapper component in
App.vue
that checks if a connection to the api can be made successfully. If that fails, it shows a dialog to configure the api url.This should allow us to ensure all api-related config (from
/info
) is fully loaded and ready to use, preventing race-conditions and hacky double checks of settings.Maybe we should integrate this here as well? That would enable redirecting the user without showing a button or other UI.
Maybe it would be enough to never set the ready state to
true
until redirected though.@ -135,3 +128,1 @@
bottom: 5vh;
color: $white;
padding: 0 1rem;
.offline-message {
unnest
Done.
@ -16,0 +4,4 @@
<div :class="{'is-hidden': !online}">
<!-- This is a workaround to get the sw to "see" the to-be-cached version of the offline background image -->
<div class="offline" style="height: 0;width: 0;"></div>
<top-navigation v-if="authUser"/>
Wrap in
<template v-if="authUser">
Why?
to make clear that it mounts at the same time than the content-auth below.
Ah, that makes sense. Done.
@ -50,2 +49,2 @@
const API_DEFAULT_PORT = 3456
import {parseURL} from 'ufo'
import {checkAndSetApiUrl} from '../../helpers/checkAndSetApiUrl'
use
@
Done.
@ -210,3 +126,1 @@
span.url {
border-bottom: 1px dashed $primary;
}
span.url {
.url
work? if not -> How about.api-url
The
.url
isn't used anywhere else. Unnesting and omitting the tag seems to work.@ -0,0 +35,4 @@
<style lang="scss" scoped>
.no-auth-wrapper {
background: url('@/assets/llama.svg') no-repeat bottom left fixed $light-background;
min-height: 100vh;
The min-height should be set from outside.
And the background as well? Because the background will only work with a height of 100vh (to keep the llama at the bottom).
Would probably be best to import the llama image as component with the new vite-svg-loader in the parent component.
Then it can be positioned absolute.
Maybe add a new issue and we resolve that after merging. Because there will be conflicts and I think that's hard to solve before.
Opened: #973
@ -72,2 +71,4 @@
},
},
props: {
configureOpen: {
I was first confused by the naming of this prop.
Since the component is just use once and if used we set this to true:
shall we just remove it to simplify this?
The
<api-config>
component is used in two places (ready and login view) and this prop is only used in one of those. Just removing the prop does not seem to be a good option here.How would you simplify it?
Can you explain why api-config is also in Login.vue? I think I didn't get that =)
Mostly to be able to change the url on the login screen, even if one is already defined. It also shows the user what api server they are connecting to.
The login screen is the first entry point, but really this should be on the other
noauth
screens as well (Register, Password reset etc).I'm planning a follow-up PR to refactor the whole noauth thing a bit, will include that there.
I'm a bit afraid that I'll never be able to merge the modals branch #816 if we don't plan ahead.
Maybe we can align those changes you still want to do or built them after merge on top of the modals branch.
Sure. It should be fine to do them after the modals branch is done.
Let's resolve this and merge this branch. Then I can try to merge it into #816 and we can think about how to continue =)
@ -75,3 +89,3 @@
setApiUrl() {
async setApiUrl() {
if (this.apiUrl === '') {
return
Explain why we return here
Done.
@ -90,2 +97,2 @@
urlToCheck = new URL(urlToCheck)
const origUrlToCheck = urlToCheck
if (url === '') {
return
Explain why we return here
@ -0,0 +15,4 @@
</div>
</template>
<script>
Use script setup
Makes a lot of sense. Done!
@ -0,0 +52,4 @@
errorNoApiUrl: ERROR_NO_API_URL,
}
},
mounted() {
start loading in created
Done.
@ -0,0 +99,4 @@
margin-bottom: 1rem;
}
.loader-container {
unnest loader-container
Done.
@ -0,0 +102,4 @@
.loader-container {
margin-right: 1rem;
&.is-loading:after {
&.is-loading::after {
(double::
)@ -19,6 +19,7 @@ import attachments from './modules/attachments'
import labels from './modules/labels'
import ListService from '../services/list'
import {checkAndSetApiUrl} from '../helpers/checkAndSetApiUrl'
Use
@
Done.
@ -3,3 +2,1 @@
position: relative;
pointer-events: none;
opacity: 0.5;
.loader-container {
how about:
Makes sense. Done.
@ -16,0 +10,4 @@
<content-no-auth v-else/>
<notification/>
</div>
<div class="app offline" v-if="!online">
Didn't test, but just from reading:
might it be that the not offline check needs to be outside of
<ready>
?Else the api might not be reached because we are offline. => slot is never exposed.
I think that's pretty much the case. Moved the offline overlay to the ready component.
I wanted to build this logic in the router via route meta fields.
There is already a super wip implementation in:
0c7df1a927
Not working right now. I basically just dumped that code in there.
@ -0,0 +65,4 @@
},
methods: {
load() {
if (window.API_URL === '') {
This error handling should happen inside the
loadApp
action.Actually, I've simplified this so that the check does not need to happen here.
@ -0,0 +72,4 @@
return !this.ready && this.error === ''
},
...mapState({
online: ONLINE,
ONLINE
is no mutation type. It's a state.Right, fixed.
@ -76,2 +89,4 @@
async setApiUrl() {
if (this.apiUrl === '') {
// Don't try to check and set an empty url
return
That seems like it should throw an error?
It does yeah. I've modified it so it shows an error with a "good" error message to the user instead of throwing one.
@ -94,1 +98,3 @@
window.API_URL = urlToCheck.toString()
if (url === '') {
// If the config setter function could not figure out a url
return
That seems like it should throw an error? (2)
@ -0,0 +2,4 @@
export const ERROR_NO_API_URL = 'noApiUrlProvided'
export const checkAndSetApiUrl = (url: string, updateConfig: () => Promise<void>): Promise<string> => {
Since
updateConfig
is always() => dispatch('config/update')
:Shouldn't we just import he store in this file and call
dispatch('config/update')
directly?Will that work outside of a vue component?
If you import the store with:
Because it's JS ™️ (trying to steal reacts claim here)
That seems like a nice way to solve it. Done.
Hi konrad!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://926-featureready-state--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!