mergesort / Boutique

✨ A magical persistence library (and so much more) for state-driven iOS and Mac apps ✨
https://build.ms/boutique/docs
MIT License
920 stars 45 forks source link

Add `init(...) async throws` overloads to the `Store`. #39

Closed rl-pavel closed 1 year ago

rl-pavel commented 1 year ago

This change adds 2 ways to observe/await for items to be loaded in the Store:

Why

I ran into an issue after implementing a thin Store dependency wrapper to use with The Composable Architecture, where I tried loading store.items on launch, but it often returned an empty array.

I believe part of the issue is that TCA seems to load @Dependeny lazily, so accessing the items right after init doesn't work. One workaround I found is to add _ = someStore to the root reducer's init. That worked better, however I worry that there might still be a race if/when the stored data grows and takes longer to load.

Compatibility

If the store was constructed in an async context, the compiler will try to use the new async version of the init and return errors about missing try and await. This could be addressed with different naming, but I'm not sure what could be used here. Thoughts? 🤔

rl-pavel commented 1 year ago

Hmm, actually it seems this doesn't really help for my use case. I saw an example of creating async dependencies with static let liveValue: @Sendable () async -> SomeDependency, but I think that would initialize a new store on every call, which is not what I want.

@mergesort I'm curious if you have any ideas/suggestions to work around the items being loaded asynchronously? Perhaps there could be a func prepareItems() async throws -> [Item] that does the work that's currently in the init, just so there's some way to ensure that the initial fetch has been completed?

mergesort commented 1 year ago

Hey @rl-pavel, thanks for the contribution, I always appreciate when someone does the research and work. 🙂

I've actually tried to tackle the idea of making init async a few times, and haven't come up with a satisfactory option. It was discussed in this thread, the biggest issues being these two issues.

  1. You can't use an async property in a property wrapper, so you can't use this Store in the @Stored property wrapper.
  2. You can't use this in a global context, so static let wouldn't work either.

I'm absolutely open to solutions, but don't want to really break the really nice API this allows for using @Stored. If there's a way to work around those issues I would definitely consider this option.

Now in terms of a workaround, I don't have a great answer for you nor do I love this but to avoid showing an empty state while the async function kicks off I've been doing is storing the latest state in a @StoredValue which is synchronous because UserDefaults gets to cheat and access data in a synchronous manner.

I will admit that it's been too long since I used TCA (especially with the new async/await APIs) that I can't answer the TCA-specific portion of your question, but hopefully that's enough information to further the conversation!

rl-pavel commented 1 year ago

@mergesort thanks the context, I have missed that thread.

Since this PR adds the init as an overload, i.e. the synchronous one with the background task still exists, I don't think this breaks the use case you're describing for @Stored? It would require a change if the store is already created in an async context, but that shouldn't be a major issue, I think. Unless I'm missing something else here?

As for my use case, I've updated the PR to store and expose the Task that loads the items so it can be awaited on. It is still run in the background in the syncronous init, and the async init version awaits for the completion at the end. What do you think of this approach?

mergesort commented 1 year ago

Hey @rl-pavel, I finally got a chance to take a closer look and I think it looks pretty good! I'll be upfront and say that I'll need to really try and use this to merge it in and have been a little underwater with work recently. In the mean time I'm going to add a little bit more information below, and some individual comments on the pull request that I think would benefit from locality.

One thing I'd love to see added if you don't mind is high level documentation about when to use the sync or async versions, along with the limitations of each approach. I know it's not a trivial amount of work, but in the context of an open source project others depend on I think it's important to do before merging in the pull request with a big context change. The places I'm thinking it would be valuable to add this to are the readme, and the two docc articles, "The @Stored Family Of Property Wrappers" and "Using Stores".

I'd especially love to see this because even as the library author I'm now having a harder time reasoning about the different combinations and possibilities, when I should be using the sync or async version (especially in the context of dropping it into an actual app). Not knowing the pros, cons, and benefits of each I can only imagine the perspective of someone coming to, which is why I think enumerating them would be really helpful.

Thanks again for the work so far, it's really neat and I appreciate you tackling this problem that I've struggled to figure out a clean solution to before!

mergesort commented 1 year ago

I apologize for any delays over the last week, I actually went on vacation a few days ago so the timing was poor, but failed to communicate that I won't be available for much of the next week with this pull request in progress.

rl-pavel commented 1 year ago

I apologize for any delays over the last week, I actually went on vacation a few days ago so the timing was poor, but failed to communicate that I won't be available for much of the next week with this pull request in progress.

All good, I actually just started a new job this week, so I might be slow to respond as well. Will try to get to your feedback this weekend!

mergesort commented 1 year ago

No rush, but I wanted to say congrats on the new job! 🥳

rl-pavel commented 1 year ago

@mergesort thank you! I have finally gotten back to tinkering with my side project and spent a bit of time on this as well. I think I've addressed most of your feedback:

One thing I'm not sure about is the documentation. As I mentioned, I've updated the comments in the code, but I haven't worked with DocC before and couldn't work out how to update the stuff there. Lmk and I'd be happy to add my notes there as well 🙂

mergesort commented 1 year ago

Thank you so much for the updates @rl-pavel! I promise that I will find the time to take a look at everything on Sunday, and assuming it all looks good (which it sounds like it should), I should be able to approve and merge it in.


Screenshot 2022-12-17 at 00 09 55

If you're familiar with markdown, working with docc is actually very similar. The only real difference is that you can hint to Xcode that you want to link to a specific type by using two backticks around a type name like so (Store), which will directly link to the Store as a result, and link directly to different docc docs with syntax like <doc:Using-Stores>.

This project is already setup for docc as you can see from the docs available at build.ms/boutique/docs. I believe the only doc that needs updating would be the one titled Using Stores, summarizing what we've been discussing here, and how to use the new async initializer. I think it would be good to add a section under Store Operations before Further Exploration, @Stored And More titled Sync or Async?, which provides the overview and context we've discussed here, guiding users to making the right choice of initializer for their needs.

I know this PR has been running on for a while and unfortunately I haven't had much time of late, but I don't want to hold up a good idea from being available to others. If you don't have time for or aren't interested in writing up the documentation I can work on it next week and add you as a reviewer to make sure I touched on everything and haven't missed anything.

Thank you again, and please let me know if anything sounds off!

rl-pavel commented 1 year ago

@mergesort no problem, and no rush, take as much time as you need 🙂

Ah, I somehow missed the Documentation file and was trying to look in the docs folder. I added a Sync or Async? section as requested (great title btw), let me know if anything could be clarified.

mergesort commented 1 year ago

Hey @rl-pavel, your pull request is great and I'm basically ready to close out this work and accept the pull request. I didn't make any code changes but I did rewrite some parts of the documentation. What you wrote was a fantastic foundation so I mostly reworded some of it to add some clarity for people who may not be as familiar with Boutique. (I also made some formatting changes, nothing major, mostly using 4 spaces over 2, moving private variables to the bottom of classes, eliminating whitespace, etc).

Your contribution was really valuable and comprehensive, I really appreciate you going the extra mile to add good documentation so others can benefit from your work. Not only am I grateful for your help and think these changes will provide a nice benefit for Boutique and it's users, but I also learned a few things in the process.

I do have one last ask before merging in the pull request. Since I rewrote some of the docs would you be able to take a quick look at what I wrote still captures the essence of your changes? I'd love to fix any mistakes I may have made, or if anything is unclear it'd be great to catch it before it goes out to users.

I just want to say again, thank you so much for all of your help. I have a few improvements I'm going to make to Boutique over the next couple of months but I think you've made a fundamentally important improvement here, and I didn't want to go without saying it means a lot to me personally, and others using Boutique.

rl-pavel commented 1 year ago

@mergesort you're welcome! And thank you, I've learned a few things along the way as well. I think your doc changes are great. Looking forward to seeing the other improvements you're talking about 👀 I'm still just getting started with Boutique, but it's been awesome to use so far, so thanks again for making this 🙂

mergesort commented 1 year ago

And it's been merged, thank you again @rl-pavel for all the help and great work! I'll integrate this into Plinky to make sure it works well enough, and then cut a new release. 😄

rl-pavel commented 1 year ago

@mergesort Awesome, thank you! Excited to see what you've been working on with Plinky. Let me know if you need any more beta testers, would love to try it out 🙂