feat(bindCallback): rename fromCallback, add tests, and fix/refactor #881
Conversation
|
👍 |
|
Cool 👍 :) |
|
It looks like it doesn't quite work properly when scheduled, too, for some reason, so I'm going to try to fix that as well. |
|
So I ended up rewriting this to use @mattpodwysocki's AsyncSubject (which might need a better name :p). This is because the observable returned has some shared state and is effectively multicast. That means that it either needs to maintain it's own list of internal subscribers, or it should use something like AsyncSubject. The former is almost definitely more performant, and we should probably move to that at some point. But the latter was convenient at this point in time. |
benlesh
changed the title
| expect(calls).toBe(0); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The Jasmine BDD style looks fairly similar to Mocha BDD. We could at some point try migrating from Jasmine to Mocha. At least we wouldn't need to rewrite/adapt every single test, it would be more a matter of fixing test-helper.js and the assertion tools for Mocha.
There was a problem hiding this comment.
I'm curious if any other uses jasmine would experience similar before making decisions of migrating to another frameworks. + if mocha does have any known issues might need to consider (course earlier would be better since there are already more than > 1000 test cases, though effort wouldn't be huge still require some..)
|
It's probably something silly with Jasmine, we can leave this here for now, and since we have something that is reproducible (hopefully) we can PR Jasmine with a resolution. |
|
So I'm going to merge this one in, but leave the weird jasmine test in that "jasmine is weird" file for now. Then if we can consistently reproduce that problem, I'll put an issue on Jasmine and try to help them resolve it. Also, those folks might be able to help us improve our test helpers or something. Perhaps that's the source of some of the weirdness. |
|
I'll also try to triage jasmine issue once this is merged. |
since fromCallback does not behave in a manner consistent with other fromX methods, it is being renamed to bindCallback as it returns a bound function that returns an observable closes #876
thisArg is removed as this method does not mirror any native function with a thisArg related #878
The previous implementation had issues with only calling the source function one time when scheduled. Since bindCallback shares its result with all subscribers, it is multicast, that means that it would need to maintain a list of subscribers internally. In leiu of that, I'm using AsyncSubject (which might need a better name) closes #881
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
since fromCallback does not behave in a manner consistent with other fromX methods, it is being renamed to bindCallback as it returns a bound function that returns an observable
closes #876