Closed kanitsharma closed 5 years ago
Thanks, this looks interesting. I will try to look it over & test it out soon.
I also want to rewrite the library in TypeScript soon, but could do that after merging this.
I will add more test cases as soon as this is merged.
@kanitsharma I should have time to review and merge this this week. Can you switch the PR to be against the dev
branch instead of the master
branch? I'd like to update that first and manually test some things out myself and then when all is good merge back to master.
@joshburgess While testing try not to use compose from @most/prelude, it has an arity of two only. i wasted a good amount on time on this :rofl: :sweat:
@kanitsharma Just giving an update here:
First, sorry for the delay. I was in Boston for a week for work + have just been generally pretty busy recently.
About the PR, I am reviewing it right now, and the example in the repo does not currently run with the above changes. I suspect that this is due to the example repo importing functions from the old version of most
and some of this code not being compatible with @most/core
.
I am going to try to update the example repo to import the equivalent functions from @most/core
to make sure everything works correctly, but I'm not sure how quickly I'll be able to do this. If you have the time to do it before me, please feel free to update your PR to include these changes.
Also, to give you more info on the previous comments about needing to make sure the Subject-like functionality still works in a synchronous way, please take a look at this closed issue:
https://github.com/joshburgess/redux-most/issues/27
It links to a code sandbox app that demonstrated a problem that was fixed in this library a while back. We will need to verify that the way you are replacing the subject usage here does not reintroduce that timing bug.
https://codesandbox.io/s/1y8rlnlp83
https://codesandbox.io/s/8n5owr4lml
I was planning to fork that code sandbox project and test the same code with your updates here, but if you have time to help, we can probably get this done more quickly.
Hey @joshburgess No problem man, thanks for informing me.
First about the timing issue, here is a fork of codesandbox with redux-most updated with @most/core
https://codesandbox.io/s/vvnyko8om5
I think there is no timing issue here, but let me know if I am wrong.
and about the examples in the repo, I will update the PR as soon as I can with the updated examples.
Also I am done with the "higher order function" PR but that is dependent of @most/core too, so I am holding it off until this is merged.
@joshburgess I have updated the example to work with @most/core :smile:
Thanks, @kanitsharma Looks good. I merged it into dev. I think I might try to rewrite this in TypeScript and maybe make some other changes also at the same time (at least rewriting some of the docs) so that I can lump everything under version 1.0.
Show me what you did with the higher order functions API idea when you're ready.