alterItems now works with an async callback function

[sean-nicholas]

Summary

alterItems did not work with a async callback function. See #367
This PR fixes #367

Documentation at: feathers-plus/docs#19

[eddyystop]

Thanks for the PR! There's a bit more to do.

Feathers is supported on NodeJS v6 and v8. v6 does not support async/await. So the actual hooks cannot use async/await anywhere.

Its preferable the tests also do not use async/await . Where that is awkward, some tests can be placed in folder tests-async and called from tests using a stub like fast-join-stub.test.js.

[sean-nicholas]

Ah okay. I will implement the changes asap :)

[sean-nicholas]

Hey, I updated the code and the tests and removed every async / await

[eddyystop]

Thank you.

[eddyystop]

In v4.12.0. Docs updated.

[bertho-zero]

This breaks my workflow with synchronous alterItems, they all turn into promises, it's a breaking change.

I think if there is no asynchronous then we should not return a promise.

EDIT: Or write on the doc that this hook now returns a promise in any case ?

[sean-nicholas]

Hmm, I think it would be cleaner to change the docs. Because different return values based on the callback might be confusing:

• Callback func returns not a promise: Don't return a promise
• Callback func returns a promise: Return a promise
• Callback func returns a promise for some records: Return a promise

@eddyystop What do you think?

PS: How do you use alterItems? If you use it as normal hook it should simply work. Do you call it yourself in a custom hook?

[eddyystop]

The hook may be called with a Promise as a param with the caller rightly expecting the hook to return a Promise. That Promise param can be resolved by the time the hook processes it. So I don't think we can use the existance of a Promise param to determine if a Promise result is returned or not.

Is there something else we can do other than having separate signatures/hooks for sync and async params?

This issue is pretty important as alterItems is likely used sync more often than async.

[sean-nicholas]

@eddyystop Don't get what you're trying to say 😄
What about this solution: https://gist.github.com/sean-nicholas/e93068b08239eac55b7140e31fb4c9c4

Be careful. I hacked it together. It does not use the hook context and therefore does not work with feathers. I just ran it in a quokka env to test it and therefore simplified the in- & output.

It now behaves as I outlined in my other comment:

• Callback func returns not a promise: Don't return a promise
• Callback func returns a promise: Return a promise
• Callback func returns a promise for some records: Return a promise

[bertho-zero]

hasPromises is the most viable solution in my opinion. (I did not test the code)

[eddyystop]

What you suggest should work. It was used at least in some old common hook code; maybe its still used.

I would suggest calculating hasPromises within the map loop.

I can fix the hook Wed or Thurs, else I can look at a PR.

[eddyystop]

@sean-nicholas, please check #409 works with your code.

[sean-nicholas]

#409 works for me 👍