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

Remove ErrorsUtil #67

Open
1 task
levibostian opened this issue Oct 15, 2019 · 2 comments
Open
1 task

Remove ErrorsUtil #67

levibostian opened this issue Oct 15, 2019 · 2 comments
Milestone

Comments

@levibostian
Copy link
Owner

levibostian commented Oct 15, 2019

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:

  • Remove the Error from OnlineDataState and LocalDataState. These structs exist to contain the state of the data, not the state of the refresh. By removing the Error from these objects, it will make these structs be closer to their purpose, make the code more simple and allow us to fix this issue.

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
Copy link
Owner Author

levibostian commented Oct 15, 2019

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 levibostian added this to the 0.6.0 milestone Nov 24, 2019
@levibostian
Copy link
Owner Author

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.

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

1 participant