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

Aborted transitions propagate error to Ember.RSVP.on('error',...) #12505

Open
olivia opened this issue Oct 20, 2015 · 42 comments
Open

Aborted transitions propagate error to Ember.RSVP.on('error',...) #12505

olivia opened this issue Oct 20, 2015 · 42 comments

Comments

@olivia
Copy link
Contributor

olivia commented Oct 20, 2015

As seen in this example, aborted transitions (through redirection or manual abortion) propagates a TransitionAborted error to the RSVP error handler while in 1.x it does not. Was this an intentional change or is the error an erroneous error?

@pixelhandler
Copy link
Contributor

@OFbriggs my best guess is that this commit - 94e1035 about 28 days ago addresses the behavior around TransitionAborted perhaps @chancancode or @rwjblue can answer this question.

@chancancode
Copy link
Member

I doubt that my commit changed that particular behavior, but I personally agree that it seems better not to propagate this to RSVP, although I am not sure what exactly has changed.

I might be looking into something related next week (routes instrumentation), so if no one else have figured it out by then I might uncover why as part of that work.

@stefanpenner
Copy link
Member

It seems like some internals are not handling the rejection, this should be considered a bug.

If the rejection is handled in the same turn, it does not propagate to on('error

@bkCDL
Copy link

bkCDL commented Nov 20, 2015

Any update on this? I experience it in Ember 2.2.0 too.

@jbryson3
Copy link

For now I'm just hardcoding the name of the error as a workaround.

export default function onServerError(cb) {
  Ember.RSVP.on('error', (reason) => {
    // An aborted transition propogates an error to RSVP
    if(reason.name !== 'TransitionAborted') {
      cb(reason);
    }
  });
}

@stianpr
Copy link

stianpr commented Feb 25, 2016

Any progress on this? We're experiencing this on Ember 2.3 as well.

@remkoboschker
Copy link

+1

@stravid
Copy link

stravid commented Mar 15, 2016

We experienced this same issue with our error tracking platform Sentry and solved it with a workaround seen in this commit. Maybe it helps someone :)

Edit: The Repository is no longer public.

@gertjanwytynck
Copy link

+1

2 similar comments
@bugduino
Copy link

+1

@jemware
Copy link

jemware commented Jul 27, 2016

+1

@devinus
Copy link
Member

devinus commented Aug 6, 2016

Thanks @stravid, this is exactly what I needed. Months of this bug and I finally found this thread and your workaround after much Google-fu. 🙇

@stravid
Copy link

stravid commented Aug 9, 2016

Thanks for the kind words @devinus, I will try to write a little blog post so in the future not so much Google-fu is needed :)

@victor95pc
Copy link

victor95pc commented Oct 25, 2016

Thanks @stravid, but it still not a solution, maybe we can add more args on the error callback to check if the model's request fail.

Boubalou added a commit to mirego/ember-new-relic that referenced this issue Nov 10, 2016
…Ember 2.X

This is a quick and dirty proposal to not log to New Relic errors that aren't desired. In the current context, there is an issue where TransitionAborted errors that are regular redirects are reported and causing a lot of noise into New Relic analytics tooling.

See emberjs/ember.js#12505 for more details on the current issue in Ember.

This proposal is inspired by the change made for Sentry by this commit: https://github.com/stravid/datsu-frontend/commit/e4d8d9adc496dff4a310f47fd95f050a71d2c196
@tchak
Copy link
Member

tchak commented Feb 28, 2017

I just merged a fix in ember-cli-sentry ember-cli-sentry/ember-cli-sentry#67

@bichotll
Copy link

Experienced it in Ember 2.5 (we are in the process to update Ember version).
For future reference, I recommend the use of ember-cli-sentry ^.

Before we found the origin of the error we even had to upgrade our sentry subscription due the amount of false alarms...plus one afternoon/evening...

oskarrough added a commit to internet4000/radio4000 that referenced this issue Oct 4, 2017
@cbou
Copy link
Contributor

cbou commented Jan 29, 2018

I also lose some time to figure out this was a false positive error.

using transitionTo inside redirect is documented in the guide and in the API:
https://emberjs.com/api/ember/2.18/classes/Route/methods/redirect?anchor=redirect

Nevertheless it still produces an error:
https://ember-twiddle.com/41c21d19e962b4981c967e46228452bb

It would save the time of a lot of emberjs new comers

@dtropp
Copy link

dtropp commented Mar 27, 2018

@chancancode or @rwjblue Planning any changes to this behaviour?
We are seeing this when we have a guard in a beforeModel() that transitions to a different route.
We are still seeing it on Ember 2.16

binoculars added a commit to binoculars/ember-osf-web that referenced this issue Apr 9, 2018
jamescdavis added a commit to CenterForOpenScience/ember-osf-web that referenced this issue Apr 9, 2018
@pixelhandler
Copy link
Contributor

@bichotll
Copy link

I barely use Ember nowadays, but I suppose you could try to reproduce it by forking this and updating the Ember version?
http://emberjs.jsbin.com/wiruqobiqe/1/edit?output

@Boubalou
Copy link

Same as @bichotll, I ended up doing backend works lately and not much into Ember anymore. I will let you guys decide whatever is good for this. :)

@bugduino
Copy link

I do not have the chance to test it

@victor95pc
Copy link

I stoped using Ember, so I cant confirm it still a issue.

@mpirio
Copy link

mpirio commented Oct 23, 2018

It seems it still an issue : https://ember-twiddle.com/fe0e87339fd079e212d5713f5288ce58

@fpauser
Copy link
Contributor

fpauser commented Oct 29, 2018

Yes, it is still an issue (tested with ember-3.5.0).

@saleswhale-vincent
Copy link

Still seeing this in 3.8

@robgarden
Copy link

@markyky
Copy link

markyky commented Jul 4, 2019

+1 on 2.18.2

@thec0keman
Copy link

For browser errors this seems like it is more of an issue of noise. However, with Fastboot this becomes a bit more difficult, since the throw will stop everything. I haven't been able to find a way to catch that particular error, and I suspect the issue is that the exception is being thrown inside of a promise error handler.

Out of curiosity, what is the value in that particular throw? I'm wondering if it could either be removed, or if the error handler could be moved so that it could be overridden.

@lucasm-iRonin
Copy link

lucasm-iRonin commented Aug 5, 2019

@Boubalou @bichotll @binoculars @bkCDL @bugduino @cbou @chancancode @devinus @dschmidt @dtropp @gertjanwytynck @jbryson3 @jemware @olivia @remkoboschker @stefanpenner @stianpr @stravid @tchak @victor95pc is this still an issue, perhaps we should close, what do you think?

@pixelhandler it's still an issue - I can reproduce it on Ember 3.8.3. I think we should consider removing inactive label, especially @robgarden provided repo for reproduction.

@Exelord
Copy link
Contributor

Exelord commented Aug 15, 2019

I agree. It's very annoying issue. Its also blocking tests if you are testing if the transition has been aborted

@James1x0
Copy link

Our catch statements for transitions that may hit an abort explicitly check for this error. Still a pain though...

@locks locks changed the title Aborted transitions in Ember 2.x propagate error to Ember.RSVP.on('error',...) Aborted transitions propagate error to Ember.RSVP.on('error',...) Jan 23, 2020
@ezpuzz
Copy link

ezpuzz commented Jan 24, 2020

Question to answer is if we ever care about TransitionAbortedError, can it happen in a way that we want the exception reported? It can be useful to know when user is getting redirected (this is usually unexpected behavior for user or bad UX, user should know where they are going when they click on something).

In many cases you should consider where the user is going before you get to redirecting them and use redirects as a last resort. Those "spurious" errors being reported are actually indications of bad UX design I think.

Of course I am affected by this issue because in many cases you don't know how to direct the user until after the model hook has fired... such as a field having changed on the backend. Still good to give user feedback about the transition instead of redirecting them silently.

That's why I think it's a good thing that they are thrown, it gives the opportunity to alert the user to an unexpected transition and remind you of places where your user experience can be improved.

@James1x0
Copy link

James1x0 commented Jan 24, 2020

@ezpuzz Tiered authorization (not authentication) is an area we always run into this. We store the transition for later so that we can provide good UX, and ease developer pain when implementing routes. We don’t know which routes need elevated permission until they are hit, in which case we abort and ask for elevated credentials before a retry. A general sweeping statement that in many cases it’s bad UX is a misnomer.

Also to note, this error is raised in an async block, not offloaded to rsvp.on(‘error’

@ezpuzz
Copy link

ezpuzz commented Jan 24, 2020

Also if you don't override RSVP.on('error') this code runs which swallows the error: https://github.com/emberjs/ember.js/blob/master/packages/@ember/-internals/runtime/lib/ext/rsvp.js

@ezpuzz
Copy link

ezpuzz commented Jan 24, 2020

@James1x0 sorry, what I meant is that there are useful cases of throwing the exception (one being identifying user pain points) but the current error doesn't include enough information right now to make it a useful bug report. In your use case you might want to record from what attempted transition the user was prompted for elevated credentials and that may happen at many possible places in your app.

In that case we could improve the usefulness of the error here: https://github.com/tildeio/router.js/blob/604f7dfa246148a7737e1bb052b563c679b6d91a/lib/router/transition-aborted-error.ts

by passing more about the transition here: https://github.com/tildeio/router.js/blob/604f7dfa246148a7737e1bb052b563c679b6d91a/lib/router/transition.ts#L401

It could be ignored otherwise as mentioned in my last comment.

hectorh30 added a commit to ServirIO/sentry-javascript that referenced this issue Jul 29, 2020
bors added a commit to rust-lang/crates.io that referenced this issue Oct 4, 2020
sentry: Ignore `TransitionAborted` errors

According to emberjs/ember.js#18416 it looks like changing query params that have `refreshModel: true` causes an internal `TransitionAborted` error. This error is being ignored by Ember itself (see https://github.com/emberjs/ember.js/blob/v3.21.3/packages/`@ember/-internals/runtime/lib/ext/rsvp.js#L40-L42),` but Sentry also hooks into `RSVP.on('error')` and unfortunately reports these false positives (see emberjs/ember.js#12505)

This PR actively ignores `TransitionAborted` errors in the `beforeSend()` hook of Sentry.

r? `@jtgeibel`
@Mifrill
Copy link

Mifrill commented Aug 29, 2021

Hi, @stravid about your message:
#12505 (comment)
The link "workaround seen in this commit." is broken:
Selection_756
can you update it, please?

@stravid
Copy link

stravid commented Aug 31, 2021

@Mifrill sorry that is not possible, I made the repository private.

@Mifrill
Copy link

Mifrill commented Aug 31, 2021

@stravid okay, no problem, can you please then post a short snippet with an example which not contains any private info

@stravid
Copy link

stravid commented Aug 31, 2021

@Mifrill here you go:

import Ember from 'ember';
import RavenLogger from 'ember-cli-sentry/services/raven';

export default RavenLogger.extend({
  ignoreError(error) {
    // Ember 2.X seems to not catch `TransitionAborted` errors caused by
    // regular redirects. We don't want these errors to show up in Sentry
    // so we have to filter them ourselfs.
    //
    // Once this issue https://github.com/emberjs/ember.js/issues/12505 is
    // resolved we can remove this code.
    return error.name === 'TransitionAborted';
  },
});

@runspired
Copy link
Contributor

still an issue in 4.x

@rtablada
Copy link
Contributor

rtablada commented Aug 8, 2022

There is a larger underlying issue here that goes beyond the commonly reported Sentry clutter. This seems to be also the root cause of ember-fastboot/ember-cli-fastboot#202 as well which has a much greater impact as it can throw 500 errors or even crash fast boot server instances.

The inconsistent error handling and async handling causes a lot of differing behavior based on the types of promises and behavior of beforeModel and afterModel.

I went in and created a reproduction repository with the different pitfalls of how beforeModel will act differently based on various promise utilizations and implementations. https://github.com/rtablada/transition-aborted-example

In the example I take the basic docs redirect example and then do some fairly common changes to the method behavior or uses which drastically vary the behavior and response how TransitionAborted is surfaced.

For best replication, you'll need to pull that repo and run it locally since I currently don't have an instance of the fast boot server running publicly.

kevinansfield added a commit to TryGhost/Ghost that referenced this issue Nov 23, 2022
no issue

- "unhandled" `TransitionAborted` errors almost always occur as part of expected application behaviour and were causing a lot of noise in Sentry making it harder to track down real errors
- when a `TransitionAborted` error occurs outside of expected behaviour it will usually be accompanied by other errors that do get logged
- there's a long-standing Ember issue about how aborted transition errors should be handled at emberjs/ember.js#12505
kevinansfield added a commit to TryGhost/Ghost that referenced this issue Nov 24, 2022
no issue

- "unhandled" `TransitionAborted` errors almost always occur as part of expected application behaviour and were causing a lot of noise in Sentry making it harder to track down real errors
- when a `TransitionAborted` error occurs outside of expected behaviour it will usually be accompanied by other errors that do get logged
- there's a long-standing Ember issue about how aborted transition errors should be handled at emberjs/ember.js#12505
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests