Closed nordfjord closed 6 years ago
@paldepind gave this example of something difficult to refactor here
streamX
.map(doSomeAsyncStuff)
.map(doMoreAsyncStuff)
.map((result) => result.done ? true : doEvenMoreAsyncStuff(result.workLeft));
An example refactor would be:
streamX
.pipe(chain(compose(fromPromise, doSomeAsyncStuff)))
.pipe(chain(compose(fromPromise, doMoreAsyncStufff)))
.pipe(chain(result => result.done
? stream(true)
: fromPromise(doEvenMoreAsyncStuff(result.workLeft))
))
What is the benefit of this?
@dmitriz commented on #166
Considering the above example
// this function returns Promise, which is not FL compliant
const getResults = filter => requestPromise('https://example.com/search?q=' + filter);
const result$ = filter$
.pipe(map(getResults))
.pipe(chain(fromPromise)) // chaining non-FL-compliant function
Would the following syntax not be simpler and FL-compliant:
// convert to FL-compliant stream right away
// a -> Stream b
const getResultsStream = filter => fromPromise(
requestPromise('https://example.com/search?q=' + filter)
)
// Get Stream b by chaining Stream a ~> (a -> Stream b) -> Stream b
const result$ = filter$
// use the ordinary monadic `chain` on Streams
.chain(getResultsStream)
first things first:
We're deprecating the .map
, .ap
, .of
methods in favour of namespaced fantasy-land ones.
so s.pipe(chain(f))
is equivalent tos['fantasy-land/chain'](f)
I agree with your premise that converting right away to stream is simpler. My main point in the example was to show in as few changed lines as possible how to convert from the old style promise swallowing to fromPromise
You could use
const result$ = filter$
- .map(getResults)
+ .pipe(chain(compose(fromPromise, getResults)));
Which is equivalent to
const result$ = filter$
- .map(getResults)
+ ['fantasy-land/chain'](compose(fromPromise, getResults));
To get a one line diff
I support this removal from the internals of streams -- if only from the angle that flyd should only be concerned with stream dependencies and updates. A module can be used if you want something like "flyd-as-promised" or similar.
Baking it into the library has the side effects you pointed out with, I'd argue, little benefit.
@nordfjord
Promise swallowing actually makes it unsafe to refactor map using the mathematical laws it's supposed to follow.
Good point. It's actually worse than just breaking the laws. It also breaks the types of the Functor map
. The type of map
should be:
map<A, B>(f: (a: A) => B, s: Stream<A>): Stream<B>;
I.e. if we map with a function that returns B
we should get a Stream<B>
back. But, if the function returns Promise<B>
we get Stream<B>
instead of Stream<Promise<B>>
.
Baking it into the library has the side effects you pointed out with, I'd argue, little benefit.
Thank you for weighing in as well @c-dante 👍 It's good to get some input before we start removing features 😉
I'm convinced that we need to get rid of the inbuilt promise handling.
But, I don't think fromPromise
is a good enough replacement.
Consider the following example:
streamX
.map(fetchNumberAsync)
.pipe(scan((sum, n) => sum + n, 0));
If I refactor the above using the approach in https://github.com/paldepind/flyd/issues/167#issuecomment-361074117 I arrive at
streamX
.pipe(chain(compose(fromPromise, fetchNumberAsync)))
.pipe(scan((sum, n) => sum + n, 0));
However, these two pieces of code do not do the same thing. There is a subtle difference (please correct me if I'm wrong here @nordfjord). If two numbers are rapidly fetched after each other then the chain
will unsubscribe from the first one and listen to the last one. This means that some numbers can get missed and that the count, in the end, migth turn out incorrect.
There are at least two different behaviors when handling promises:
fromPromise
and chain
gives.The fast that we can support the first behavior is a really good thing. But if we remove promise shallowing we need a good migration story for people who currently rely on the last behavior as well.
I think we need a function like this (maybe not with that name though).
function flattenPromise<A>(stream: Stream<Promise<A>>): Stream<A> { ... }
The function should behave so that streamX.map(fnAsync)
with the current promise shallowing is identical to flattenPromise(streamX.map(fnAsync))
.
With this function the example in https://github.com/paldepind/flyd/issues/167#issuecomment-361074117 becomes:
streamX
.map(doSomeAsyncStuff)
.pipe(flattenPromise)
.map(doMoreAsyncStuff)
.pipe(flattenPromise)
.map((result) => result.done ? true : doEvenMoreAsyncStuff(result.workLeft));
.pipe(flattenPromise)
It's still slightly more code but is probably an easier refactor to than the other one.
What do you think about that?
Agreed, I didn't think about the case when ordering doesn't matter and promises can't be dropped. The flattenPromise utility would be trivial to implement.
// flattenPromise :: Stream (a | Promise a) -> Stream a
const flattenPromise = promise$ => {
const s = stream();
promise$.map(promise => {
// debatable whether we need this
// maybe we should force people to explicitly
// return promises from the stream
if (isPromise(promise)) promise.then(s);
else s(promise);
});
return s;
}
I'm purposefully omitting end stream mechanisms in the example above.
We could simplify the migration a bit as well by utilising compose
// mapPromise :: (a -> (b | Promise b)) -> Stream a -> Stream b
const mapPromise = compose(flattenPromise, map);
streamX
.pipe(mapPromise(doSomeAsyncStuff))
.pipe(mapPromise(doMoreAsyncStuff))
.pipe(mapPromise(result => result.done
// if we need to explicitly return promises then:
// ? Promise.resolve(true)
? true
: doEvenMoreAsyncStuff(result.workLeft)
))
@nordfjord
It seems that we agree 😺 I'm wondering a bit out how we should remove promise shallowing. We could deprecate it and log errors like you did in #166. That would work. But the types would also be wrong and the promise shallowing would still be there with its problems. Alternatively, we could just remove it an release a new major version?
We could also merge #166 as it is now and then immediately afterward create a new major release with promise handling removed and with flattenPromise
and fromPromise
?
I think you nailed it with releasing deprecations as 0.2.x and immediately release a new major release.
This gives users the new features of #166 without breaking existing code, and allows them to transition fairly quickly to a new major version, they can even use the typescript types as helpers in the migration process.
Hey @paldepind
I've been thinking about this for the past few days.
Is it absolutely necessary to include both flattenPromise
and fromPromise
?
I think we should make an effort to solve the 99% use case in the core and maybe add flattenPromise
as a flyd module.
I think having two methods to turn a promise into a stream in core is going to be confusing for users.
What do you think?
@nordfjord I'm not sure I agree that having both is confusing. They do different things that are both "reasonable"? If properly documented I think people will be able to figure it out 😄 In fact I think they are quite different. One of them turns Promise<A>
into Stream<A>
and the other turns Stream<Promise<A>>
into Stream<A>
.
But I'm not opposed to having one of them in a module. Maybe we could even have both in a module? Another alternative for Flyd is to adopt ES modules. Then we could export all modules from one file and tree-shaking (as of Webpack 4) would take care of removing any unused modules.
I'd like to make a few points about promise swallowing and how it is actually a harmful feature.
It breaks
map
map
should follow some lawsf.map(g).map(f)
is equivalent tof.map(compose(f,g))
f.map(d => d)
is equivalent tof
Now let's try those laws out with promises
Promise swallowing actually makes it unsafe to refactor
map
using the mathematical laws it's supposed to follow.It breaks ordering
Imagine the same example as above
Since the code handling promises in flyd is just
No ordering is guaranteed. So if I quickly write some filter like: 'res', then 3 promises are generated and they can be resolved in any order, the stream will always contain the last resolved promise.
How do we fix these issues
It's simple, remove promise swallowing in favour of a
fromPromise
methodlet's take the same example and rewrite it using the
fromPromise
helperNow what has changed?
map