levibostian / Teller-iOS

iOS library that manages your app's cached data with ease.
MIT License
13 stars 0 forks source link

Remove ErrorsUtil #67

Open levibostian opened 5 years ago

levibostian commented 5 years ago

The class ErrorsUtil is used to compare errors for classes that conform to Equatable. (1) That's hacky to compare Error for equatable, (2) it's not up to us to conform Error to Equatable and (3) I don't trust it. When using a hack, it gives you an opportunity to step back and ask if you can do something better.

Tasks:

To the end user they may not notice much. We may change the API to whenCache {} and whenNoCache {} where the errors do not exist in there and instead move that logic into the result returned from the Observable. We could change OnlineRepository.observe() to return a tuple, a new struct, or a Result<> type instead that houses an error.

This will remove some hackiness from the library, make the API easier to work with when observing the cache.

levibostian commented 5 years ago

This could go another step further. Instead of removing just the Error from the data state objects, we could have Repository.observe() return a tuple with (DataState, RefreshResult) where the refreshing is entirely separated.

When I work with Teller, I have found myself often doing:

observe()
.subscribe { dataState
  if dataState.isFetching() {
    // show in UI data is fetching
  }
  data.whenCache {}
  data.whenNoCache {}
}

So, I handle the data differently from the UI anyway. Also, I have worked with people the past few months who use Teller in their apps. When I watch them, they get confused by the Repository.observe() API where they are wondering what noCache() and cache() means in the DataState object. By separating the concerns, that should be more digestible where you understand what Teller is doing more now.

This would also solve the bug that we have right now where the OnlineRepository will have the state of data be "cache empty" right after the first fetch is successful and then half a second later the cache state gets updated to the real state of the cache (if empty or not empty). If we separated the state of the cache and the fetching status, that would make this better where this quick flash between the 2 would not exist.

Also, it has been discussed in the future to create a "Teller-UI" library. Teller is heavy on the backend of things but it is also meant to work with a UI. That's the whole purpose: to show cached data in a UI. Therefore, we could create protocols that views can extend such as, RefreshResultUIPresentor so then you can easily do:

observe()
.subscribe { dataState, refreshResult in 
  refreshResult.presentIn(view)
}

To make re-useable Views and code. Easier to test views too where you can test the states.

levibostian commented 4 years ago

With this change, I also hope that [we can remove DataState.none state.

That state exists mostly because the fetching status and cache status need to be coupled together. With it being split apart, it may not be needed any longer.