-
Notifications
You must be signed in to change notification settings - Fork 14
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
@most/core #32
@most/core #32
Conversation
kanitsharma
commented
Sep 25, 2018
- Removed Most-Subject from dependencies
- Removed Most 1.0 from peer dependencies
- Replaced subjects with multicast streams
- Replaced most 1.0 functions with @most/core's functions
- Updated tests to work with @most/core
- All tests are passing.
- I tested the updated code in a project of mine and it ran fine 😄 but more testing should be needed.
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 |
@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 🤣 😓 |
…rt interop with observables
@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 I am going to try to update the example repo to import the equivalent functions from 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: 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 😄 |
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. |