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

Promise.reduce(array, mapFn, initialValue) #46

Closed
oltrep opened this issue Nov 19, 2020 · 6 comments · Fixed by #47
Closed

Promise.reduce(array, mapFn, initialValue) #46

oltrep opened this issue Nov 19, 2020 · 6 comments · Fixed by #47

Comments

@oltrep
Copy link
Contributor

oltrep commented Nov 19, 2020

I encountered a use case where I need to do an array reduce operation with a callback that may return a promise. When the reduce callback returns the first promise, the reduction will continue after each promise resolves.

If that's a feature you would like for this library, I can submit a pull request for it!

@evaera
Copy link
Owner

evaera commented Nov 19, 2020

This seems like it might unintentionally create waterfalls (sequential execution instead of concurrent)? Is this the only way that your function could run? Would it be possible to throw a Promise.all somewhere before this?

@evaera
Copy link
Owner

evaera commented Nov 19, 2020

I feel mixed on this. Assuming you had a reduce function from another library (like cryo or something), could you just throw an :expect() at the end of the Promise to turn it into a yielding call?

@oltrep
Copy link
Contributor Author

oltrep commented Nov 19, 2020

This is a better explanation of the feature: http://bluebirdjs.com/docs/api/promise.reduce.html

It is sequential because of the reduce operation, but the advantage is that it returns a promise instead of yielding until the actual result.

@jhampton
Copy link

Correct me if I'm wrong @oltrep, but this is also has the benefit of a single reject or resolve while preserving iteration order (being important for cases when there is a promise -> result -> input to next promise set of use cases).

Also, this is a great library @evaera, thank you so much! Do you feel an :expect() is more idiomatic or is inline with your existing implementations?

@evaera
Copy link
Owner

evaera commented Nov 20, 2020

I see! I think this can be useful. My only reservation would be inadvertently encouraging people to reach for the Promise library to do common operations like reduce or map, which would incur a lot of unnecessary overhead, rather than using a library like Cryo or similar which doesn't have extra machinery for handling asynchronous code.

That said, I think that when used in the right scenario, an abstraction like this will save time and prevent mistakes.

I imagine it could be implemented over the existing Promise.each function, something like this:

function Promise.fold(list, predicate, initialValue)
  local accumulator = initialValue
  return Promise.each(list, function(value)
    accumulator = predicate(value, accumulator)
 end):andThenReturn(accumulator)
end

I think I prefer the name Promise.fold, because we accept an initial value rather than use the first element of the list as the default value, and the name fold is used by other existing Roblox Lua libraries that perform the same function.

If you submit a PR, please:

  • Update the CHANGELOG, putting your changes under a # Next header
  • Add tests for the function with reasonable cases
  • Update the documentation YAML file and add the function (sorry, this is a little tedious!)

Thanks!

@evaera
Copy link
Owner

evaera commented Nov 20, 2020

Also, this is a great library @evaera, thank you so much!

Thank you! 😄

@oltrep oltrep mentioned this issue Nov 20, 2020
@evaera evaera closed this as completed in #47 Dec 2, 2020
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 a pull request may close this issue.

3 participants