feat: use script setup and ts in app auth components #1160

Merged
dpschen merged 1 commits from dpschen/frontend:feature/feat-ts-and-setup-in-app-auth into main 2021-12-12 14:37:21 +00:00
Member
No description provided.
dpschen force-pushed feature/feat-ts-and-setup-in-app-auth from 074e3713d1 to b833555511 2021-12-08 13:06:10 +00:00 Compare
Member

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1160-featurefeat-ts-and-setup-in-app-auth--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!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1160-featurefeat-ts-and-setup-in-app-auth--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! > Beep boop, I'm a bot.
dpschen changed title from WIP: feat: use script setup and ts in app auth components to feat: use script setup and ts in app auth components 2021-12-09 17:15:11 +00:00
dpschen requested review from konrad 2021-12-09 17:15:16 +00:00
konrad was assigned by dpschen 2021-12-09 17:15:19 +00:00
konrad reviewed 2021-12-11 12:56:08 +00:00
konrad left a comment
Owner

Looks good, only some nits.

Looks good, only some nits.
src/App.vue Outdated
@ -24,0 +21,4 @@
import {useRoute, useRouter} from 'vue-router'
import {useStore} from 'vuex'
import {useI18n} from 'vue-i18n'
import { useOnline } from '@vueuse/core'

Please make it consistent with the rest:

-import { useOnline } from '@vueuse/core'
+import {useOnline} from '@vueuse/core'
Please make it consistent with the rest: ```diff -import { useOnline } from '@vueuse/core' +import {useOnline} from '@vueuse/core' ```
Author
Member

I kind of ignored this kind of formatting for now since this can be automated with prettier / eslint.

I kind of ignored this kind of formatting for now since this can be automated with prettier / eslint.

That's fine too. Another reason to integrate prettier :)

That's fine too. Another reason to integrate prettier :)
Author
Member

Changed it anyway for now

Changed it anyway for now
dpschen marked this conversation as resolved
src/App.vue Outdated
@ -113,0 +47,4 @@
const router = useRouter()
// Make sure to always load the home route when running with electron
if (route.fullPath.endsWith('frontend/index.html')) {

I'm not even sure if we need this anymore. Will check.

I'm not even sure if we need this anymore. Will check.

We definitely don't need this. Could you remove it?

We definitely don't need this. Could you remove it?
Author
Member

Removed

Removed
dpschen marked this conversation as resolved
@ -132,0 +86,4 @@
// TODO: Reset the title if the page component does not set one itself
function useRenewTokenOnFocus() {

This looks like an excellent candidate to be moved in a seperate file and imported from there. Could be another PR though.

This looks like an excellent candidate to be moved in a seperate file and imported from there. Could be another PR though.
Author
Member

Yes for example.

I wanted to move the files to script setup because this makes refactoring easier =)

So this does for sure not solve everything for auth

Yes for example. I wanted to move the files to script setup because this makes refactoring easier =) So this does for sure not solve everything for auth
dpschen marked this conversation as resolved
dpschen force-pushed feature/feat-ts-and-setup-in-app-auth from b833555511 to c89ceb7e38 2021-12-12 12:25:13 +00:00 Compare
dpschen force-pushed feature/feat-ts-and-setup-in-app-auth from c89ceb7e38 to 2643b62fd7 2021-12-12 12:55:04 +00:00 Compare
dpschen reviewed 2021-12-12 12:56:53 +00:00
src/App.vue Outdated
@ -122,0 +55,4 @@
const {t} = useI18n()
// setup account deletion verification
const accountDeletionConfirm = useRouteQuery('accountDeletionConfirm') as Ref<null | string>
Author
Member

Not sure about this typing 🤷‍♂️ but seems to work.

Not sure about this typing 🤷‍♂️ but seems to work.
konrad marked this conversation as resolved
dpschen force-pushed feature/feat-ts-and-setup-in-app-auth from 2643b62fd7 to 3813a0c388 2021-12-12 12:59:53 +00:00 Compare
dpschen reviewed 2021-12-12 13:00:51 +00:00
@ -23,6 +23,7 @@
"@sentry/vue": "6.16.1",
"@vue/compat": "3.2.26",
"@vueuse/core": "7.3.0",
"@vueuse/router": "^7.3.0",
Author
Member

This package is super slim so I felt there is no problem to add. Would have written something similar but probably worse either way.

This package is super slim so I felt there is no problem to add. Would have written something similar but probably worse either way.

yeah should be fine.

yeah should be fine.
konrad marked this conversation as resolved
dpschen reviewed 2021-12-12 13:02:52 +00:00
@ -123,0 +56,4 @@
// setup account deletion verification
const accountDeletionConfirm = useRouteQuery('accountDeletionConfirm') as Ref<null | string>
watch(accountDeletionConfirm, async (accountDeletionConfirm) => {
Author
Member

When converting these functions to watchers in 807fb6a9fe the initial call got removed. I readded it by adding immediate: true.
I hope this was correct.

When converting these functions to watchers in 807fb6a9fe404f65f0616aac78f707cd6e4ade5d the initial call got removed. I readded it by adding `immediate: true`. I hope this was correct.

I only added the watcher because the initial call did not work at all. If your solution works without a watcher that should be fine.

I only added the watcher because the initial call did not work at all. If your solution works without a watcher that should be fine.
Author
Member

It is still a watcher but just calls on initial setup. Didn't test if it works though, but I don't know why it shouldn't.

It is still a watcher but just calls on initial setup. Didn't test if it works though, but I don't know why it shouldn't.

Just tested it, works great.

Just tested it, works great.
konrad marked this conversation as resolved
Author
Member

@konrad Can you recheck this?

@konrad Can you recheck this?
konrad reviewed 2021-12-12 13:42:13 +00:00
@ -46,0 +14,4 @@
const route = useRoute()
watchEffect(() => {

Doesn't this need immediate: true as well?

Doesn't this need `immediate: true` as well?
Author
Member
[watchEffect runs immediate by default](https://v3.vuejs.org/guide/reactivity-computed-watchers.html#watcheffect)
dpschen marked this conversation as resolved
@ -98,0 +112,4 @@
errorMessage,
password,
authenticate,
} = useAuth()

Any special reason to put this in a function and creating const from it instead of not using a function and declaring the variables directly?

Any special reason to put this in a function and creating const from it instead of not using a function and declaring the variables directly?
Author
Member

😆 I also felt that this is very verbose.

The reason was that I wanted to prepare abstraction of the authentification part.

Since this pull request is mostly about converting to script setup + ts this is probably one little extra setup too much (the function wrapper).

😆 I also felt that this is very verbose. The reason was that I wanted to prepare abstraction of the authentification part. Since this pull request is mostly about converting to script setup + ts this is probably one little extra setup too much (the function wrapper).

That makes sense.

That makes sense.
konrad marked this conversation as resolved
konrad approved these changes 2021-12-12 13:58:50 +00:00
konrad left a comment
Owner

LGTM, just waiting for the CI to pass.

LGTM, just waiting for the CI to pass.
dpschen merged commit c3c4d2a0a5 into main 2021-12-12 14:37:21 +00:00
dpschen deleted branch feature/feat-ts-and-setup-in-app-auth 2021-12-12 14:37:21 +00:00
Author
Member

🥳

🥳
This repo is archived. You cannot comment on pull requests.
No description provided.