vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

feature/vue3-global-error-management #833

Closed
dpschen wants to merge 2 commits from feature/vue3-global-error-management into vue3
pull from: feature/vue3-global-error-management
merge into: vikunja:vue3
Conversation 1 Commits 2 Files Changed 75 +199 -601
dpschen commented 2021-10-12 14:05:08 +00:00
Member
Copy Link

This pull request changes the way errors are handled. Basically I removed all the default error handlers and replaced them with one global as long as they didn't have a specific error message (then they stay). Not 100% convinced that is something that should stay.

Later I might need to readd some catch blocks. My idea was to create something like a "sentry suuper lite". So no backend or anything. But throw and show every error there is to be able to find out if it's important or not.

I think that's important because it might be that there are still some errors in the application from the vue 3 transition that I'm not aware of because they get catched somewhere silently.

This pull request changes the way errors are handled. Basically I removed all the default error handlers and replaced them with one global as long as they didn't have a specific error message (then they stay). Not 100% convinced that is something that should stay. Later I might need to readd some `catch` blocks. My idea was to create something like a "[sentry](https://sentry.io/) suuper lite". So no backend or anything. But throw and show every error there is to be able to find out if it's important or not. I think that's important because it might be that there are still some errors in the application from the vue 3 transition that I'm not aware of because they get catched somewhere silently.
dpschen added 28 commits 2021-10-12 14:05:10 +00:00
continuous-integration/drone/push Build is failing Details
36d4599276 fix: unassign user success messgage (#831) 
Co-authored-by: Dominik Pschenitschni <mail@celement.de>
Reviewed-on: #831
Reviewed-by: konrad <k@knt.li>
Co-authored-by: dpschen <dpschen@noreply.kolaente.de>
Co-committed-by: dpschen <dpschen@noreply.kolaente.de>
continuous-integration/drone/push Build is passing Details
3c5c3cad10 chore: remove unneeded babel packages and add peerDependencies (#828) 
Co-authored-by: Dominik Pschenitschni <mail@celement.de>
Reviewed-on: #828
Reviewed-by: konrad <k@knt.li>
Co-authored-by: dpschen <dpschen@noreply.kolaente.de>
Co-committed-by: dpschen <dpschen@noreply.kolaente.de>
continuous-integration/drone/pr Build is passing Details
continuous-integration/drone/push Build is passing Details
17d791027c
feat: add legacy build
35c861b711
feat: create randomId helper and use everywhere 
Co-authored-by: konrad <konrad@kola-entertainments.de>
continuous-integration/drone/pr Build is passing Details
fd77aaa123
fix: add timeout to wait for move to finish
continuous-integration/drone/pr Build is passing Details
18d7ca0820
feat: also use createRandomID helper in editor
continuous-integration/drone/pr Build is passing Details
5cfb99bfc2
Merge branch 'main' into feature/vue3-implementation-improvements 
# Conflicts:
#	package.json
#	vite.config.js
#	yarn.lock
d1b61a1489
feat: some vue3 package improvements 
- add codemirror as direct dependency
- remove @vue/runtime-dom (was needed for volar with vue2)
- remove setting for using volar with vue2
continuous-integration/drone/pr Build is failing Details
18c31482df
fix: remove side effect from computed 
was firing a commit
continuous-integration/drone/pr Build is failing Details
cc8b03778c
fix: use correct method for fakers uuid
continuous-integration/drone/pr Build is passing Details
50fa592aad
fix: wrong async order
dpschen added a new dependency 2021-10-12 14:05:29 +00:00
#814 feature/vue3-implementation-improvements
dpschen added a new dependency 2021-10-12 14:13:00 +00:00
#832 feature/vue3-async-await
dpschen force-pushed feature/vue3-global-error-management from c3ba3c58bc to d4fed645b4 2021-10-15 18:12:15 +00:00 Compare
dpschen changed title from WIP: feature/vue3-global-error-management to feature/vue3-global-error-management 2021-10-15 18:31:19 +00:00
dpschen requested review from konrad 2021-10-15 18:31:25 +00:00
dpschen added the
kind/feature
 label 2021-10-15 18:31:48 +00:00
dpschen removed a dependency 2021-10-15 18:32:01 +00:00
#814 feature/vue3-implementation-improvements
dpschen force-pushed feature/vue3-global-error-management from d4fed645b4 to 571c69311e 2021-10-15 18:49:34 +00:00 Compare
konrad reviewed 2021-10-15 21:31:48 +00:00
src/main.ts
@ -99,2 +99,3 @@
console.error(err, vm, info)
// console.error(err, vm, info)
error(err)
// }
konrad commented 2021-10-15 21:24:23 +00:00
Owner

Can you remove the commented code? (not 100% sure if you've already done this somewhere else, in that case please ignore my comment)

Can you remove the commented code? (not 100% sure if you've already done this somewhere else, in that case please ignore my comment)
👍 1
dpschen marked this conversation as resolved
src/services/abstractService.js Outdated
@ -446,3 +419,3 @@
* @returns {Q.Promise<any>}
*/
delete(model) {
async delete(model) {
konrad commented 2021-10-15 21:25:48 +00:00
Owner

If the delete method is async, shouldn' the rest also be async?

Or are they already by nature of returning the call to the http... methods?

If the delete method is async, shouldn' the rest also be async? Or are they already by nature of returning the call to the `http...` methods?
dpschen commented 2021-10-16 09:53:24 +00:00
Author
Member

As long as they always return a promise they are already async – which should be the case if they return the call of a async function like axios.

Most of the time you add async to a function if you use await directly.

As long as they always return a promise they are already async – which should be the case if they return the call of a async function like axios. Most of the time you add async to a function if you use `await` directly.
dpschen marked this conversation as resolved
src/store/modules/labels.js Outdated
@ -105,3 +97,1 @@
})
.catch(e => Promise.reject(e))
.finally(() => cancel())
try {
konrad commented 2021-10-15 21:30:02 +00:00
Owner

Any special reason to use try .. finally over then().finally()? I think both are fine, just curious. I think we should decide on one style and stick with it.

Any special reason to use `try .. finally` over `then().finally()`? I think both are fine, just curious. I think we should decide on one style and stick with it.
dpschen commented 2021-10-16 10:04:37 +00:00
Author
Member

async … await with try … catch is the way to go.

There are some cases where I prefere .then() because I think the syntax is better:

const deepProp = await apiService().then(({prop}) => prop.deepProp)

instead of

const prop = await apiService()
const { deepProp } = prop

Because async/await in the end is syntactic sugor over promises sometimes you really need to use promises instead. E.g. here you need to use promises because a constructor cannot be async.

By the way: there was an issue: reminderDates was just filled in the async callback of the promise. But the promise was just necessary for the notifications – not for the creation of the reminderDates.

`async` … `await` with `try` … `catch` [is the way to go](https://dev.to/gafi/7-reasons-to-always-use-async-await-over-plain-promises-tutorial-4ej9?ref=hackernoon.com). There are some cases where I prefere `.then()` because I think the syntax is better: ```js const deepProp = await apiService().then(({prop}) => prop.deepProp) ``` instead of ```js const prop = await apiService() const { deepProp } = prop ``` Because `async`/`await` in the end is syntactic sugor over promises sometimes you really need to use promises instead. E.g. [here](https://kolaente.dev/dpschen/frontend/src/branch/feature/vue3-global-error-management/src/models/task.js#L28) you need to use promises because a constructor cannot be async. By the way: there was an issue: [`reminderDates` was just filled in the async callback of the promise](https://kolaente.dev/dpschen/frontend/src/branch/feature/vue3-async-await/src/models/task.js#L27). But the promise was just necessary for the notifications – not for the creation of the `reminderDates`.
konrad commented 2021-10-16 10:29:40 +00:00
Owner

I see, thanks for clarifying. The article you linked to makes a lot of sense.

I see, thanks for clarifying. The article you linked to makes a lot of sense.
konrad marked this conversation as resolved
dpschen force-pushed feature/vue3-global-error-management from 571c69311e to b10e14546d 2021-10-16 13:24:16 +00:00 Compare
dpschen force-pushed feature/vue3-global-error-management from b10e14546d to 30d6b7e6fc 2021-10-16 15:57:14 +00:00 Compare
dpschen force-pushed feature/vue3-global-error-management from 85967e7a9a to a776e1d2f3 2021-10-16 17:00:27 +00:00 Compare
dpschen commented 2021-10-16 18:49:21 +00:00
Author
Member
Copy Link

I'll close this in favor of #832.
Since I'm developing there already either way and the commmits are included I think that should be fine.

I'll close this in favor of https://kolaente.dev/vikunja/frontend/pulls/832. Since I'm developing there already either way and the commmits are included I think that should be fine.
dpschen closed this pull request 2021-10-16 18:49:21 +00:00
dpschen removed a dependency 2021-10-16 18:49:42 +00:00
#832 feature/vue3-async-await
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
konrad
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#833
No description provided.
Delete Branch "feature/vue3-global-error-management"

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?