Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to trigger error messages without the template? #76

Closed
BenJackGill opened this issue Jul 6, 2022 · 8 comments
Closed

How to trigger error messages without the template? #76

BenJackGill opened this issue Jul 6, 2022 · 8 comments

Comments

@BenJackGill
Copy link

BenJackGill commented Jul 6, 2022

I am trying to use Vue Concurrency with my Vue 3 / TypeScript / Quasar app.

Previously, I was handling the display of Task error messages in the template with a v-if like this:

<script setup lang="ts">
import { useTask, timeout } from 'vue-concurrency';
const throwError = async () => {
  await timeout(1); // Simulate async
  throw new Error('Ruh oh! Error.');
};
const exampleTask = useTask(function* () {
  yield throwError;
});
</script>

<template>
  <div v-if="exampleTask.last?.isError">
    {{ exampleTask.last?.error }}
  </div>
  <q-btn label="Run Task" @click="exampleTask.perform"></q-btn>
</template>

But now I am using Quasar Notify to display error messages in a popup. These popups are not written into the template. Instead they are triggered from the script, like this:

import { useQuasar } from 'quasar';
const $q = useQuasar();
const triggerNotification = (errorMessage: string) => {
  $q.notify(errorMessage);
};

But I don't know how to connect the error generated by the Task to the triggerNotification function.

To give a final overview, this is where the code currently stands, without knowing how to correctly connect the error generated by the Task to the triggerNotification function:

<script setup lang="ts">
import { useTask, timeout } from 'vue-concurrency';
import { useQuasar } from 'quasar';
const $q = useQuasar();
const triggerNotification = (errorMessage: string) => {
  $q.notify(errorMessage);
};
const throwError = async () => {
  await timeout(1);
  throw new Error('Ruh oh! Error.');
};
const exampleTask = useTask(function* () {
  yield throwError;
});
</script>

<template>
  <q-btn label="Run Task" @click="exampleTask.perform"></q-btn>
</template>

Do you have any guidance on how to get this working?

@BenJackGill BenJackGill changed the title How to handle error messages without the template? How to trigger error messages without the template? Jul 6, 2022
@BenJackGill
Copy link
Author

BenJackGill commented Jul 6, 2022

Ok I figured out I can chain a catch() on the end of every yield statement, like this:

<script setup lang="ts">
import { useTask, timeout } from 'vue-concurrency';
import { useQuasar } from 'quasar';
const $q = useQuasar();
const triggerNotification = (errorMessage: string) => {
  $q.notify(errorMessage);
};
const throwError = async () => {
  await timeout(1);
  throw new Error('Ruh oh! Error.');
};
const exampleTask = useTask(function* () {
  yield throwError().catch((err) => {
    triggerNotification(err.message);
  });
});
</script>

<template>
  <q-btn label="Run Task" @click="exampleTask.perform"></q-btn>
</template>

With this example it seems simple, but in real life I have many Tasks and each contains many yield statements within.

Do I have to chain a catch() on the end of each yield statement in every Task?

That looks messy and verbose.

I thought I could implement a useTaskGroup and chain a single catch() on the end of that. But it that doesn't work because useTaskGroup is not "thenable".

Maybe there is another solution where I can group together all the tasks and apply one catch() at the end?

@MartinMalinda
Copy link
Owner

hi @BenJackGill

in real life I have many Tasks and each contains many yield statements within.

I think this depends on how you want to handle the errors. I sometimes use these popup notifications as well - they're good for cases when there's no relevant place to display the error, and especially if something breaks in the background.

So my workflow usally is 90% times - display error "in place" and use task.last.error. In the rest cases I do try catch inside the task and use something like your triggerNotification.

Buf if you plan to use triggerNotification a lot, it's indeed maybe not optimal to do so many try catch.

What you can do is wrap the entire task perform in a try catch.

<script setup lang="ts">
import { useTask, timeout } from 'vue-concurrency';
import { useQuasar } from 'quasar';
const $q = useQuasar();
const throwError = async () => {
  await timeout(1);
  throw new Error('Ruh oh! Error.');
};
const exampleTask = useTask(function* () {
  yield throwError().catch((err) => {
    triggerNotification(err.message);
  });
});

const save = async () => {
  try {
   await exampleTask.perform();
  } catch (e) {
    $q.notify(error.message);
}
};
</script>

<template>
  <q-btn label="Run Task" @click="save"></q-btn>
</template>

You'd be creating these wrapper functions a lot, so you can write some higher order function for it:

const performAndNotify = async (task, ...args) => {
  try {
  await task.perform(...args);
 } catch (error) {
  $q.notify(error.message);
 }
};

Then you can reuse this function at multiple places.

@BenJackGill
Copy link
Author

BenJackGill commented Jul 20, 2022

Sorry for my late reply, I was hit with Covid, but am better now.

Thanks for such a thoughtful answer. It was very helpful.

I think I will do as you say, and keep 90% of error messages in place instead of putting them all into a popup notification.

@MartinMalinda
Copy link
Owner

Great!:)

@TRScheel
Copy link
Contributor

TRScheel commented Aug 3, 2022

Would you be opposed to including event handlers as optional parameters? For example, here in TaskInstance.ts line 217:

if (e !== "cancel") {
        taskInstance.error = e;
}

If that also called any optionally attached event handlers then outside functions could hook in and request updates upon errors. Additionally, could define a global event handler for onError so one part of someone's application could receive notifications upon failures anywhere in their application.

If you're not opposed to the above I might spend some time implementing it and submitting a PR for it.

@MartinMalinda
Copy link
Owner

MartinMalinda commented Aug 3, 2022

I think passing a callback like onError to one specific task could be a little bit antipattern. If you need that you probably want to have a wrapper function with

const myAction = () => {
  try {
   task.perform()
  } catch (e) {
  
  }
}

It's a bit more verbose but it enforces better logic flow I think.

But the global onError function is much needed I think. For proper error reporting to Cloud Services and other global error handling.

I also proposed it here:
#50 (comment)

Just from the top of my mind I'm not sure how to implement that. That functions has to be store somewhere. A lot of other plugins use the vue app instance for that that would require additional setup of app.use(vueConcurrency). But maybe there's a smarter way to achieve this.

PR or any ideas for this welcome!

@TRScheel
Copy link
Contributor

TRScheel commented Aug 3, 2022

It will likely be two PRs then, one for the global and another for the local.

I can see advantages to the local scoped event handler and even some use cases for it but I understand the issues that might arise from that pattern.

@MartinMalinda
Copy link
Owner

ok, sounds good 👍 for the local one it would be good to add a disclaimer to the docs not to overuse it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants