Future #432
Future #432
Conversation
|
ping @nechaido |
| } | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Could this perhaps be rewritten without double Future?
map(fn) {
return new Future((resolve, reject) => {
this.run(value => {
try {
resolve(fn(value));
} catch (error) {
reject(error);
}
});
});
There was a problem hiding this comment.
Thanks for bringing the attention here!
I was able to spot the inconsistency of error handling.
The ideologically correct way to implement map function for any monad is the application of chain to the composition of of and fn. Rambda did it.
To do it here one simply needs to move error checking from here to run method.
There was a problem hiding this comment.
Implementation of map through already existed chain and of is less efficient because of the creation of two Futures. That said such implementation reuses code and is the same for any monad. Though the duplicated code here is tiny meaning it may be allowed here for clarity.
So there is a tradeoff between performance and ideological purity here :-)
|
|
||
| promise() { | ||
| return new Promise((resolve, reject) => { | ||
| this.run(value => resolve(value), error => reject(error)); |
There was a problem hiding this comment.
| this.run(value => resolve(value), error => reject(error)); | |
| this.run(value => resolve(value), reject); |
There was a problem hiding this comment.
I think the lambda is used for dropping other arguments
|
|
||
| chain(fn) { | ||
| return new Future((resolve, reject) => | ||
| this.run(value => fn(value).run(resolve, reject), error => reject(error)) |
There was a problem hiding this comment.
| this.run(value => fn(value).run(resolve, reject), error => reject(error)) | |
| this.run(value => fn(value).run(resolve, reject), reject) |
| const { Future, futurify } = require('..'); | ||
| const metatests = require('metatests'); | ||
|
|
||
| metatests.test('Future map/run', async test => { |
There was a problem hiding this comment.
| metatests.test('Future map/run', async test => { | |
| metatests.test('Future map/run', test => { |
?
(and in other tests)
|
|
||
| metatests.test('Future stateless', async test => { | ||
| const f1 = Future.of(3); | ||
| const f2 = f1.map(x => ++x); |
There was a problem hiding this comment.
| const f2 = f1.map(x => ++x); | |
| const f2 = f1.map(x => x + 1); |
| .map(() => { | ||
| throw new Error('msg'); | ||
| }) | ||
| .run(test.mustNotCall, error => { |
There was a problem hiding this comment.
| .run(test.mustNotCall, error => { | |
| .run(test.mustNotCall(), error => { |
| } | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Thanks for bringing the attention here!
I was able to spot the inconsistency of error handling.
The ideologically correct way to implement map function for any monad is the application of chain to the composition of of and fn. Rambda did it.
To do it here one simply needs to move error checking from here to run method.
| } | ||
|
|
||
| run(successed, failed) { | ||
| this.executor(successed, failed); |
There was a problem hiding this comment.
I think it's important to catch errors the executor may throw. It will also allow omitting error handling in the map method
| } | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Implementation of map through already existed chain and of is less efficient because of the creation of two Futures. That said such implementation reuses code and is the same for any monad. Though the duplicated code here is tiny meaning it may be allowed here for clarity.
So there is a tradeoff between performance and ideological purity here :-)
Closes: #431