Skip to content
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

Fix bug where sampleOn updates before both trigger and source streams #108

Closed
wants to merge 1 commit into from
Closed

Conversation

alexgig
Copy link

@alexgig alexgig commented Apr 4, 2016

The commit includes the fix + tests.

Best,
Alex

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 992e55a on fluid-ui:master into 5753f0e on paldepind:master.

@alexgig
Copy link
Author

alexgig commented Apr 21, 2016

Hi @paldepind,

This fix addresses a bug that arises when the source stream depends on the trigger stream.

Currently, the sampleOn stream will take the source value when trigger emits but BEFORE the source stream has been updated as a result of the trigger stream emitting.

After the fix, first the trigger emits, then the source gets updated in reaction to trigger emitting, and finally sampleOn samples from the source stream in reaction to trigger emitting.

Can we get this merged? Let me know if you need anything else from me.

Thanks,
Alex

@coveralls
Copy link

coveralls commented May 6, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling fd3d44b on fluid-ui:master into ca1cc08 on paldepind:master.

@@ -1,7 +1,8 @@
var flyd = require('../../lib');
var R = require('ramda');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulling in the entire ramda library as a dependency seems kind of unecessary here, since you only use it for R.contains. You might saw the ramda dependency in package.json but only ramda/src/curryN is actually used in the flyd source. This would increase the bundle size by quite a bunch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @queckezz. The latest commit only brings in contains.

@coveralls
Copy link

coveralls commented May 11, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 9d8768c on fluid-ui:master into ca1cc08 on paldepind:master.

@alexgig alexgig closed this Jun 10, 2016
@paldepind
Copy link
Owner

Hello @alexgig. I missed this PR. Sorry 😞

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.

4 participants