-
Notifications
You must be signed in to change notification settings - Fork 34
Jenn/threadsafe enhancer #78
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
Jenn/threadsafe enhancer #78
Conversation
|
|
||
| * @returns {StoreEnhancer} A store enhancer that synchronizes the store. | ||
| */ | ||
| fun <State> synchronizeStore(): StoreEnhancer<State> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more descriptive name such as createSynchronizedStoreEnhancer or syncronizedStoreEnhancer to make it clear this creates and returns a StoreEnhancer and update docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patjackson52 updated name to createSynchronizedStoreEnhancer
616e8bf to
704a71b
Compare
|
I removed the failing test that was used to communicate the issue with |
|
@jennkao ktlint is failing with some unused imports. |
`createSynchronizedStoreEnhancer` generates a store enhancer that wraps a Redux store in a synchronization object, causing access to store methods to be synchronized. Recommended usecase is when using a thread-safe store with enhancers or middleware that require access to store methods.
704a71b to
fa1230b
Compare
fa1230b to
d2b6e15
Compare
|
hey @patjackson52 - following up here, do I need to do anything to get the checks to rerun? |
|
@jennkao merged and the snapshots are published. Would you be interested in making a PR to bump the version so we can publish release artifacts? |
|
@patjackson52 yeah sure thing! opened a PR here - #91 |
Following up on issue #77, this PR creates a new enhancer called
synchronizeStorethat wraps a Redux store in a synchronization object.I wrote a couple tests to compare the original thread-safe store implementation that we were working with against the proposed method of using the
synchronizeStoreenhancer. The test involves thunk middleware as well as delays to mimic async thunks, and the test for the original implementation intermittently fails with the same error we were seeing in our setup. Notably, when using a breakpoint on line 105 of theCreateThreadSafeStoreSpecfile and running the test for the original implementation in debug mode, I see that we step into the interface defs of the original store methods inCreateStore; doing the same with the test for the enhancer implementation sees us step into the interface defs of the synchronized store inSynchronizedStore. This suggests to me that with the original implementation, thunks might be accessing the unsynchronized storedispatchmethod.The failing test was written just for communicating the issue we were seeing, happy to remove this and/or change up the tests as you see fit. This is my first time contributing to this repo so please let me know if I missed anything in terms of contributing standards. Thanks!