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

feat: update angular service mapping #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JosefBredereck
Copy link

While I was watching your last two videos:

I wondered if it was possible to skip the subscriptions and work out something differently. I would like to hear your opinion on that matter.

Therefore, I added an idea that simply maps all server requests into a unified result object, which will always start with a loading indicator and give me the appropriate error as soon as any error occurs.

Filter Changes

Further, I changed the behavior of the filter because the filtering mostly happens on the server and will resolve the result with filtered entries.

An extension would be to have the API tell us how many pages could be found for the given filter, and we would always start at page 1 as soon as the filter changed.

Refreshing

The idea of refreshing in case of an error was cool to see, but I often tend to have the requirement to refresh data as it could have been updated on the server. And to give the service the possibility of doing both, I changed the refresh behavior.

Pagination

I did not understand why the current page needed to live in the state. Therefore, I changed that too. If you completely want to prevent the usage of BehaviorSubject, there is also a way to archive this via subject. But this would be far more complicated for the use case.

@joshuamorony
Copy link
Owner

Thanks for taking the time to create this PR! In general I think your approach is good and I don't have any problems with it. Typically I would also prefer to not have subscribes.

But the goal here for me was to make some sacrifices in order to simplify things (such that only a minimal amount of RxJS knowledge would be required) but also keep things reasonably reactive/declarative. I like that the approach in the video has a reasonably simple and consistent concept: next sources when something happens, have a bit of code that decides how to update the state, all state is selected from a single state object.

I feel with an approach closer to yours that the complexity is increased a bit, and we lose the simplicity of the sort of "single source of truth" state object, and you're probably going to need to pull in some more RxJS knowledge to get things working (which is fine, but one of my aims was simplicity/teachability). For example, if you wanted to include more state than just the articles in the state object (e.g. the currentPage), then you're probably going to have to do some more combineLatest stuff with toSignal, which is fine, but I feel like it would become too intimidating for people not already familiar with RxJS. It would probably be better to just not have the centralised state object in this case anyway I think.

Thanks again, interactions like this from the videos is what I hope for!

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.

2 participants