-
Notifications
You must be signed in to change notification settings - Fork 84
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
Flip argument order of takeUntil module #175
base: master
Are you sure you want to change the base?
Flip argument order of takeUntil module #175
Conversation
913d659
to
46ef88c
Compare
This would close #121 |
module/takeuntil/README.md
Outdated
``` | ||
|
||
__Signature__ | ||
|
||
`Stream a -> Stream b -> Stream a` | ||
`Stream b -> Stream a -> Stream a` |
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.
Maybe we should flip the meaning of a
and b
instead of flipping their order? As in Stream a -> Stream b -> Stream b
?
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.
Ah! yes of course! that makes much more sense, nice catch!
Good observation. I agree. The new argument order is more natural for currying 👍 |
e490924
to
a9e1471
Compare
I think this would qualify as a breaking change, so I think we might need to bump the version to 0.3.x |
a9e1471
to
c7f2e16
Compare
The takeUntil module violates one of the first principles of functional programming. Its argument order was (src, end) which is a data first argument order. This commit flips the argument order to (end, src) allowing us to better use takeUntil in pipelines such as: ```js const query = stream(''); // Start fetching the results but if they haven't // arrived when a new query is emitted // stop listening to the stream const results = query .chain(q => fromPromise(getResults(q)) .pipe(takeUntil(query)) ) ``` This also fixes an issue with takeUntil where if the new endStream had a value the dependent stream wouldn't update.
c7f2e16
to
96c915c
Compare
var drop1 = flyd.transduce(drop(1)); | ||
|
||
module.exports = flyd.curryN(2, function(term, src) { | ||
var end$ = flyd.merge(term.hasVal ? drop1(term) : term, src.end); |
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.
To maintain the behaviour of takeUntil as it was before #180 we need to drop a single value from the terminator stream if it already has a value, otherwise the stream will end immediately.
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.
an example of code that wouldn't work without it would be:
const s = stream();
const result$ = s.chain(q => fromPromise(getSearchResults(q)))
.pipe(takeUntil(s))
.map(results => h('ul', results.content.map(res => h('li', res))))
.map(render('#results'));
In the above example you would only be able to search once. Any subsequent search would be ended immediately and not return a value.
The takeUntil module violates one of the first principles of functional
programming. Its argument order was (src, end) which is a data first
argument order.
This PR flips the argument order to (end, src) allowing us to better
use takeUntil in pipelines such as:
Which could also be written as
If you prefer compose to dot-chains.