Skip to content

Conversation

@iampatbrown
Copy link
Contributor

@iampatbrown iampatbrown commented Aug 29, 2021

This is a proof of concept as a way to provide consistent ordering for StorePublisher subscribers (discussed in #698). @mbrandonw mentioned:

It would be great if it could be fixed at the library level

so I thought I'd give it a try. Pretty sure it will need to be made thread safe and have some extensive tests/benchmarks added.

I can see the benefits of sticking with CurrentValueSubject and I haven't thoroughly tested this... just checking to see if it's something worth exploring further. I used CombineExt/CurrentValueRelay as a starting point.

@stephencelis
Copy link
Member

@iampatbrown Thanks for the PR! @mbrandonw and I just chatted about it and think it's a simple enough addition that we think it's worth merging, especially if it makes things a bit more deterministic than the tools Apple ships. The existing benchmarks (simple as they are) don't appear to be negatively affected.

We also think that thread safety is probably not necessary due to the fact that it is only used in the store, which is already designed to not be thread safe.

If you think this is ready, please feel free to remove the draft status and we'll look to merge!

@iampatbrown
Copy link
Contributor Author

We also think that thread safety is probably not necessary due to the fact that it is only used in the store, which is already designed to not be thread safe.

I thought this might have been the case as well. My main concern was if people were using viewStore.publisher.subscribe(on:). Had a feeling this might be impacted. But yeah, the docs are pretty clear about not being thread safe though. I guess we can revert back fairly easily if there are any problems.

Any preference on the file structure and naming?

@stephencelis
Copy link
Member

Ha didn't mean to leave you hanging here!

Any preference on the file structure and naming?

Nope! What you have looks great. If you think this is ready, please move it into review and we'll get it merged 😄

@iampatbrown
Copy link
Contributor Author

@stephencelis Hahaha. No worries. Did you give it a test drive? Pretty sure it was all good when we originally looked at it.

@iampatbrown iampatbrown marked this pull request as ready for review September 17, 2021 19:38
@stephencelis
Copy link
Member

@iampatbrown Yup! All looked good to us and seems to benchmark about the same 😄

@stephencelis stephencelis merged commit 4b8db86 into pointfreeco:main Sep 17, 2021
mbrandonw added a commit that referenced this pull request Sep 20, 2021
* Replaced CurrentValueSubject with CurrentValueRelay

* Added final to DemandBuffer

Co-authored-by: Brandon Williams <mbrandonw@hey.com>
mbrandonw added a commit that referenced this pull request Sep 20, 2021
* Perform thread check only when store is created on main thread.

* clean up

* Update Sources/ComposableArchitecture/Store.swift

* clean up

* Update Sources/ComposableArchitecture/Store.swift

* clean up

* execute setSpecific only once.

* logic fix

* added a test

* typo

* wip

* docs

* fix test

* Update Sources/ComposableArchitecture/Store.swift

Co-authored-by: Thomas Grapperon <35562418+tgrapperon@users.noreply.github.com>

* Run swift-format

* Clean up speech recognition case study. (#812)

* Clean up speech recognition case study.

* fix tests

* clean up;

* Alternative to `CurrentValueSubject` in `ViewStore` (#755)

* Replaced CurrentValueSubject with CurrentValueRelay

* Added final to DemandBuffer

Co-authored-by: Brandon Williams <mbrandonw@hey.com>

* Run swift-format

* Fix bindable deprecations (#815)

* Fix Bindable Deprecations

* More CI

* wip

* wip

* wip

* wip

* Run swift-format

* beef up test

* expectation

* fix

Co-authored-by: Stephen Celis <stephen@stephencelis.com>
Co-authored-by: Thomas Grapperon <35562418+tgrapperon@users.noreply.github.com>
Co-authored-by: stephencelis <stephencelis@users.noreply.github.com>
Co-authored-by: iampatbrown <mrpatbrown@gmail.com>
Co-authored-by: mbrandonw <mbrandonw@users.noreply.github.com>
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.

3 participants