-
Notifications
You must be signed in to change notification settings - Fork 102
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
Allow fn passed to Async.fromNode to be partially applied #478
base: master
Are you sure you want to change the base?
Conversation
6765d2c
to
de54156
Compare
69bc2ea
to
079e279
Compare
@evilsoft @dalefrancis88 Just letting you know that this one is now ready for review 🙂 |
079e279
to
99f6bd2
Compare
Thanks mate, i'll get some 👀on this tomorrow or overmorrow |
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.
My knowledge around Async
is limited, I don't use it. So i believe @evilsoft will need to add context where i've made uninformed statements
@JamieDixon Reviewed with some questions but this is looking good
const _fn = curry(fn) | ||
|
||
return (...args) => { | ||
|
||
if (args.length < _fn.length - 1) { | ||
return fromNode(_fn(...args), ctx) | ||
} | ||
|
||
return Async((reject, resolve) => { |
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.
@evilsoft I know there is the issue with Async
swallowing errors in one particular scenario, for this change, will wrapping this in the curry
and the recursive call to fromNode
create a similiar scenario?
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.
Nah, we should be good with this.
The Async "issue" is actually the desired behavior when dealing with Promises
. And it is a behavior of Promises, so we should be good here!
if (args.length < _fn.length - 1) { | ||
return fromNode(_fn(...args), ctx) | ||
} |
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.
Per your documentation and our discussions their is a chance that the given function has a length of 0. Would this be a case to return an error?
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.
My understanding at this point is that if the function has a length of 0 then the safest thing to do is assume that it isn't being partially applied, and apply all of the parameters as per usual. This will mean that any code that currently passes a compose
d or ...args
or argument
function will continue to work in exactly the same way as it does today.
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.
That's true, sounds good to me
docs/src/pages/docs/crocks/Async.md
Outdated
good candidates for partial application. | ||
|
||
<sup>[1]</sup> The final parameter to the incoming function is provided to you by | ||
`fromNode` rather than being part of the parameters that can be partially applied. |
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.
We should avoid starting and ending lines with 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.
Thanks! Fixed 😃
… applied in some cases
99f6bd2
to
030c299
Compare
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.
Got a chance to play with this today and found something I think we should address before this is merged in. One of the goals with crocks
is to have the currying "just work". With reliance on the length
property, we run into some unexpected behavior when we have something like this:
// fn :: Number -> Number -> NodeCB -> ()
const fn = curry(
target => (value, cb) => {
target >= value
? cb(null, target - value)
: cb({ message: 'not valid' }, null)
}
)
Notice how the curried function is of: Number -> (Number, NodeCB) -> ()
, this should curry out just fine like:
// logIt :: NodeCB
const logIt = (err, data) => {
log({ err, data })
}
fn(10)(9)(logIt)
//=> { err: null, data: 1 }
fn(10, 19, logIt)
//=> { err: { message: 'not valid' }, data: null }
So, when this curried function is applied to fromNode
, it returns the Async
and gets into a state where it never fires the callback to resolve/reject the Async.
// desired siggy
// asyncFn :: Number -> Number -> Async Err Number
const asyncFn =
fromNode(fn)
asyncFn(10)
.fork(rej => log({ rej }), res => log({ res }))
// returns an Async that is stuck in a "pending" state
// should return a function, ready for the 'value' argument
Will come up with a couple suggestions, but wanted to let you know what I found
Async.fromNode
returns a function that eventually resolves to anAsync
type once it has been applied to all of its parameters.This PR is a first-pass at allowing the function returned from
Async.fromNode
to be partially applied.Dependent on Expose a length property for curried functions #477Note: In its current form this PR will not pass CI until #477 is merged.