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

refactor(nav): Exception in lifecycle will print stack trace #10974

Closed
wants to merge 2 commits into from
Closed

refactor(nav): Exception in lifecycle will print stack trace #10974

wants to merge 2 commits into from

Conversation

royipressburger
Copy link

Short description of what this resolves:

Now its very hard to tell what is the error and where it is coming from because the _lifecycle method doesn't print the exception stacktrace.

Changes proposed in this pull request:

-Just added console.log

Ionic Version: 1.x / 2.x
2

Now its very hard to tell what is the error and where it is coming from because the `_lifecycle` method doesn't print the exception stacktrace.
@AmitMY
Copy link
Contributor

AmitMY commented Mar 30, 2017

Perhaps, instead of fully catching the error in the lifecycle event, what do you think of re-throwing that error, so the global error handler could catch it? (IonicErrorHandler as default, but for example, I use rollbar, and this solution will also report to my rollbar account, not just have an error I wouldn't know about)

btw, please review the commit message style next time you submit a PR.
https://github.com/driftyco/ionic/blob/master/.github/CONTRIBUTING.md#commit-message-format

@royipressburger royipressburger changed the title Exception in lifecycle will print stack trace type(refactor): Exception in lifecycle will print stack trace Mar 30, 2017
@royipressburger
Copy link
Author

You are right. Looks like a better solution.

@royipressburger royipressburger changed the title type(refactor): Exception in lifecycle will print stack trace refactor(nav): Exception in lifecycle will print stack trace Mar 30, 2017
@manucorporat
Copy link
Contributor

I am not sure this is a good solution. The point of having a try/catch is to isolate NavController from the user's code.

If the exception is rethrow, it could lead NavController to a inconsistent state.

@royipressburger
Copy link
Author

@manucorporat Then what will be the right way to see the stack trace when there is exception in the lifecycle methods?

@AmitMY
Copy link
Contributor

AmitMY commented Mar 30, 2017

@manucorporat While I understand where you are coming from, the _lifecycle method is executing user code, for example, if I write:

ionViewWillEnter() {
    throw new Error("you will never catch me");
}

If I understand that code correctly, this error will be catched by that try/catch, which is not the intended behaviour imo.

Funny story - When I tested rollbar for the first time with my app, I threw an error from the ionViewWillEnter, or another event - can't really remember - and I just didn't understand why it didn't work.. Moved to a different method, and it did. I think now I know why, my error was catched by something outside of my ErrorHandler's control

@manucorporat
Copy link
Contributor

I totally agree with you @AmitMY but rethrowing the exception is not an option.
We actually have interesting options, for example, we could inject the abstract class ErrorHandler (that references an instance of IonicErrorHandler).

This way we could show nice the error output in screen without actually breaking NavController.

@AmitMY
Copy link
Contributor

AmitMY commented Mar 31, 2017

@manucorporat The way I see it, because the NavController is an angular service, if you rethrow the error, the global handler would catch it.
If that is not the case, then sure, get the ErrorHandler's instance somehow, and use the handleError method..

@manucorporat
Copy link
Contributor

that's not the problem. the problem is that lifecycle events are dispatched while a transition is ocurring, throwing an exception would interrupt the normal flow of a transition leaving internal variables in an inconsistent state.

@manucorporat
Copy link
Contributor

@AmitMY @royipressburger fixed here: 95c06a5

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

Successfully merging this pull request may close these issues.

3 participants