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

[wip] Allow safe handling of Routable calls #59

Closed
wants to merge 7 commits into from
Closed

[wip] Allow safe handling of Routable calls #59

wants to merge 7 commits into from

Conversation

cuteiosdev
Copy link

@cuteiosdev cuteiosdev commented Mar 10, 2017

The problem encountered

Rather unfortunately our app gets itself into a state whereby we try to perform a push, pop or change action on a route that isn't supported by a routable. This often results in fatalError being called and the app crashing.

A possible solution

This is an idea / attempt at fixing the problem though is unfortunately incomplete. By calling a function defined on the routable before calling push, pop or change we can check whether the action will in fact work. This is a much more desirable result than a fatalError.

Unfortunately by having the check in the Router which is a store subscriber, it occurs after the navigation state has been updated. This means that we prevent the call resulting in fatalError but we have a state that is not reflective of where the app is.

As such, there's a need to also have a ReSwift Middleware written in the app in order to check whether we can navigate to a route before the state is changed.

A call for help / suggestions

It would be great to get some discussion and input happening around this change. I'm not 100% sure what the best approach here in fact is.

@codecov-io
Copy link

codecov-io commented Mar 10, 2017

Codecov Report

Merging #59 into master will decrease coverage by 9.07%.
The diff coverage is 23.63%.

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   80.38%   71.31%   -9.08%     
==========================================
  Files           5        5              
  Lines         209      244      +35     
==========================================
+ Hits          168      174       +6     
- Misses         41       70      +29
Impacted Files Coverage Δ
ReSwiftRouter/Router.swift 71.66% <22.44%> (-11.12%)
ReSwiftRouter/Routable.swift 16.66% <33.33%> (+16.66%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a18c2f...3ace5d6. Read the comment docs.

@cuteiosdev
Copy link
Author

@Ben-G I've just rebased this branch. Could you take a look and see whether anything has been accidentally clobbered?

@Ben-G
Copy link
Member

Ben-G commented Mar 10, 2017

@mattdelves I'm curious to understand why the following happens:

Rather unfortunately our app gets itself into a state whereby we try to perform a push, pop or change action on a route that isn't supported by a routable. This often results in fatalError being called and the app crashing.

Crashing in that case seems appropriate, as the declared route and the actual route no longer match up; a situation from which the router probably cannot recover. Really this shouldn't happen in the first place so I would be interested in finding and hopefully fixing the root cause.

@cuteiosdev
Copy link
Author

@Ben-G yeah, completely agree that the best solution is to look for why we are ending up with a route that isn't achievable.

Unfortunately, the situation in which our app is in means that that isn't always easy to find out. The desired outcome here is preventing a state that results in a crash. This should be achievable by the changes above and the use of a middleware written in the app.

Hope that clears up why we're wanting to make use of this change.

@Ben-G
Copy link
Member

Ben-G commented Mar 15, 2017

@mattdelves While I definitely understand how this is causing issues in your app, I don't think we should change the router API based on that. The hard failure via fatalError is intended to make developers aware of inconsistent routing early on so that they can fix the issue quickly. The changes here could lead to subtle navigation issues that devs wouldn't be aware off.

That said; it seems like you should be able to implement the ideas from this PR via extensions inside of your codebase to workaround your immediate problems?

@cuteiosdev
Copy link
Author

@Ben-G those are some good points. Unfortunately even when implementing the middleware component in the app we had to commit some rather heinous crimes with regard to the code.

As such, since this is likely not something that'll be part of the API, I'm going to close the pr.

@cuteiosdev cuteiosdev closed this Mar 15, 2017
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.

None yet

4 participants