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

assertionFailure() as opposed to fatalError() #85

Open
viewDidAppear opened this issue Oct 19, 2017 · 3 comments
Open

assertionFailure() as opposed to fatalError() #85

viewDidAppear opened this issue Oct 19, 2017 · 3 comments

Comments

@viewDidAppear
Copy link

Hello,

In my own fork of this project, I've gone ahead to remove all references to fatalError() in my use of the Router. This warranted a change to the router itself, to prevent any operations from occurring if I ever return nil from the pushRouteSegment or changeRouteSegment functions. I have manually popped the route from the stack if we ever hit newState() with an invalid route.

Is this the correct way to do this? I would like some further clarification too on why fatalError() was chosen as opposed to assertionFailure() which would be evaluated in DEBUG builds only. The situation we faced was the occasional (widespread) crash in the Routables in our release builds which was horrendously detrimental to our user experience.

I am happy to make a pull request for this too. Otherwise a discussion would be great! 🙌

@jondwillis
Copy link

jondwillis commented Oct 31, 2017

The situation we faced was the occasional (widespread) crash in the Routables in our release builds which was horrendously detrimental to our user experience.

The general problem with replacing fatalError() with assertionFailure() is that you are replacing a crash, which comes with accompanying debug information to help you fix the crash, with undefined behavior. That undefined behavior could be just as bad or worse than a crash from a UX perspective, and you'll have no idea when it is happening to your users.

@DivineDominion
Copy link
Contributor

I don't use the Router myself, so I wonder what kinds of situations make you run into the fatalError conditions in the live app. At best, this should not happen at all. @jondwillis point is valid, so maybe it'd be better to change the approach of the Router in general if it is error-prone than just removing the crashes.

@viewDidAppear
Copy link
Author

Well it concerned Routables which didn’t always implement all screens. And the function expected to return a routable and so we’d hit our fatalError()

The way I ended up doing it was blocking any operation if canPush/canPop returned false etc. seems to work well.

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