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

Provide an API to declaratively catch certain errors, bubble others #40

Open
machty opened this issue Mar 31, 2016 · 10 comments
Open

Provide an API to declaratively catch certain errors, bubble others #40

machty opened this issue Mar 31, 2016 · 10 comments

Comments

@machty
Copy link
Owner

@machty machty commented Mar 31, 2016

No description provided.

@sukima
Copy link

@sukima sukima commented Jul 21, 2016

For reference, Ember's PromiseProxyMixin has the exact same problem.

@sukima
Copy link

@sukima sukima commented Jul 21, 2016

Thoughts on how this might look API wise. Since the intent of ember-concurrency is to leverage as much of the JavaScript language as possible I think a decorator might be the way to go:

import Ember from 'ember';
import { task, didCancel, handledError } from 'ember-concurrency';

export default Ember.Component.extend({
  myTask: task(function * () {
    try {
      yield Ember.RSVP.reject('bork');
    } catch(reason) {
      if (!(didCancel(reason))) {
        throw handledError(reason);
      }
    }
    return 'foobar';
  })
});

In this manor the myTask.last.error will be the reason but it won't bubble to Ember's unhandled rejection code because it was wrapped in handledError(). If it was not wrapped then it would bubble as default.

This would let the coder perform tests on the rejection reason:

if (error instanceof MyExpectedError) {
  throw handledError(err);
} else {
  throw error;
}

rethrowing the handled error means the template can still use myTask.last.error without worry of it reaching Ember's onerror.

@machty
Copy link
Owner Author

@machty machty commented Jul 21, 2016

@sukima recent versions of e-c don't "throw" cancelation errors, so there's no need to use didCancel() here. didCancel() only exists for when you treat a performed task like a promise (you use .then() or .catch()). Since promises don't (yet) have a notion of cancelation, you're forced to explicitly handle cancelation requests in the .catch() handler, which is why you need didCancel. More details on this here

Honestly this issue isn't as problematic as it used to be now that we have the new behavior of cancelation "errors" skipping catch{} blocks. At this point any error handling awkwardness is just as awkward as plain ol JavaScript, and improving the error handling story isn't as high a priority on the ember-concurrency roadmap.

@sukima
Copy link

@sukima sukima commented Jul 21, 2016

Even with the improved cancellation behavior wouldn't the user still have the problem of differentiating between an expected error (for example a 422 validation errors from a server) and an unexpected one (i.e. programming/runtime error)?

I would like to explicitly mark an exception object as one that is intended to be managed by me (like displayed in the template) and leave the unmarked ones to ember-concurrency who would bubble it up to Ember.onerror so my sentry / logging server can notify me.

Is this something worth looking into? Or should I not add it to my weekend hobby list.

@machty
Copy link
Owner Author

@machty machty commented Jul 21, 2016

Well definitely feel free to explore and experiment, but my hunch is that e-c might not be the library to try and improve the error handling semantics that plague JS in general. On the other hand, if a general purpose pattern catches on in the Ember community in general I'd be happy to bring support to e-c where it makes sense.

@sukima
Copy link

@sukima sukima commented Jun 7, 2017

I found a possible workaround. In your Ember.onerror function have a trap that checks to see if the exception has a handled property set to true.

Usage

Ember.onerror = function(error) {
  if (Ember.get(error, 'handled')) {
    return; // Do nothing
  }
  // Do your normal error handling
  console.error('Unhandled exception', error);
};

Consumption

myTask: task(function * () {
  try {
    doSomething();
  } catch (error) {
    error.handled = true;
    throw error;
  }
})

And now {{myTask.last.error}} works as expected.

@sukima
Copy link

@sukima sukima commented Jun 7, 2017

For example I am doing something like this in my app:

locateMe: task(function * () {
  const geoLocation = get(this, 'geoLocation');
  try {
    let result = geoLocation.getPosition();
    set(this, 'boundingBoxValue', this.boundingBoxAround(result));
  } catch (error) {
    if (isGeoLocationError(error)) {
      error.message = geoLocation.translatedMessageForError(error);
      error.handled = true;
    }
    throw error;
  }
}).drop()
@sukima
Copy link

@sukima sukima commented Feb 3, 2018

For reference I have found a workaround for this issue but does mean the responsibility is pushed to the caller instead of the task itself. I would like to propose a task modifier that does this as part of core.

performTask() {
  let taskInstance = this.get('myTask').perform();
  // Declare my intent to handle the error through myTask.error
  // no-op the default unhandled error handler.
  taskInstance.catch(() => {});
  return taskInstance;
},
myTask: task(function * () {
  yield timeout(1000);
  throw new Error('Bork!');
})
@machty machty mentioned this issue Mar 20, 2018
1 of 3 tasks complete
@hrishikeshs
Copy link

@hrishikeshs hrishikeshs commented Sep 30, 2019

Hi, I'm facing the same issue. I have a choice between doing

try {
  this.myTask.perform();
} catch(e) {
...
}

versus

this.myTask.perform().catch()

is there any reason to not use this.myTask.perform().catch() ? the documentation on the website almost exclusively uses try..catch

@sukima
Copy link

@sukima sukima commented Oct 4, 2019

Using try/catch is more expressive and easier to reason about. It also allows the cancellation/error handling to remain in the task. Using .catch() will down cast the TaskInstance to just a Promise and now you have to manage everything manually yourself since you have left safety and comfort of ember-concurrency

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

Successfully merging a pull request may close this issue.

None yet
4 participants