promise.all for request transforms

[skibz]

let me know what you think 😄
from issue

[skellock]

Wow! Nice! In my brain, it looked a lot more complicated.

One side effect this has is that we now run these in parallel. The order might matter to some people.

Also, adding the async keyword will need us to bring in a new babel transform as we ship in ES5 mode. And now with promises, we'll need to provide instructions for polyfilling on older browsers.

I'm ok with all that stuff, just thinking outloud.

Thank you kindly for adding the tests. I'll take a closer look this weekend!

[skibz]

thanks for the feedback!

agreed on the note of serial transform execution. how do you think this would be best handled?

[skellock]

Maybe two phases.

Phase 1 is what we have now (pre-PR). We synchronously loop. Phase 2 is what you're proposing, a Promise.all free for all.

Have the guts just run them back to back? I'd be down for that.

[craigcartmell]

Any news on this?

I'm attempting to get a token from AsyncStorage in the addRequestTransform method but it's not working out for me.

// Force Authorization header on all requests
  api.addRequestTransform(async request => {
    const jwt = await AsyncStorage.getItem(jwtStoreKey)

    if (jwt) {
      request.headers.Authorization = String(jwt)
    }
  })

Any ideas on a workaround in the meantime before this is fully resolved or should I attempt to use this branch?

[skellock]

The thing that's holding me back on this is, we're going to have to do a few things:

1. Introduce a breaking API change unless we have 2nd set of transform for the async flavour.
2. Introduce another babel transform for environments that aren't React... unless we just say, "Bring your own Promise polyfill"... which i'm kinda ok with.

If you do checkout this branch, lemme know how it goes in your app! Might help alleviate my concerns with this.

[craigcartmell]

No problem, I'll give it a whirl. This must be a fairly common API use-case though, getting a token from storage and sending it in a request?

Is there another way I could accomplish this using apisauce?

[skellock]

Not sure of your set up there, but one option is to create the api every request. Some kind of factory to set it all up. Part of that setup process, you'd go out to AsyncStorage and grab that key.

The call to this factory function would be async.

Essentially creating an stateless version.

Which is really great... and the cost of a small performance it. (which would be trivial since we're talking to the internet!).

[skellock]

This async feature should be a thing though. I want it too! :)

[craigcartmell]

Good idea re: the factory, cheers!

[craigcartmell]

This is working perfectly for me in my react-native project (based on the Ignite framework). Nice work @skibz.

@skellock: How about adding it as a separate method as oppose to adding the functionality into addRequestTransform as per this PR?

Maybe a simple addAsyncRequestTransform to keep it backwards compatible?

Then as you say, the user can implement their own polyfill if they want to take advantage of it.

[skellock]

Ya, I'm down with that!

[skellock]

I seem to be having an issue with this. The transforms aren't being applied serially. I think we want to keep the same intention which is chaining. We just want to do it async. Lemme know if I'm wrong here.

[craigcartmell]

I'm not sure I completely follow. Could you post an example of what you have currently?

[skellock]

Sorry, I didn't explain that well at all. Ha!

This implementation is parallel + fast fail. All transforms would be run parallel and when they all pass, control flow continues. Should any fail, they all get rejected.

A ...
B ...
C .......
         ... continue

The way current transforms work is that they happened first-come first-serve with swallowed errors.

A ...
     B ...
          C ...
               ... continue

It's not that this new way is wrong, it's just that it's not like how it currently works.

Actually this implementation is both faster and more inline with people expect with promises.

So I'll kick this back to you guys...

Is this what you're looking for?

Should we have two different async types?

.addSerialRequestTransform( x => x )
.addParallelRequestTransform( y => y)

or

.addRequestTransformAsync( x => x ) // default serial
.addRequestTransformAsync( y => y, true ) // opt-in parallel

I think once we lock this down, I'll call this 1.0.

[craigcartmell]

Ah, I see now.

Default to serial and opt in flag looks good to me!

[skibz]

hey @skellock

sorry for taking so long to update all of this!

i've changed this branch to use addAsyncRequestTransform function. doRequest will now execute asynchronous transforms first and will not execute synchronous transforms until asynchronous ones are complete.

i see that you've begun discussing different ways of operating (opt-in parallel and such) so i'm not sure if you all would be satisfied with what i've done 😄

to test the async request transforms, run:

npm i mocha
./node_modules/.bin/mocha --compilers js:babel-core/register test/mocha-asyncRequestTransformTests.js

i added test cases with mocha because the ava test suite was hanging for more than four to five minutes at a time (i have a positively diminutive computer 😅 )

so please let me know if the ava tests are broken as i cannot run them!

[skellock]

Thanks for all this work! I appreciate it.

ava will kick the crap out of your computer. That's a feature! But it shouldn't take 4-5 minutes. What happens if you run ava -s? This turns off the parallel support and tests are executed serially.

The promise.all call will run things in parallel, but I believe serial should be the default.

Once I get a block of contiguous time (hopefully this week), I'll merge this in and make a few changes.

Thank you!

[skibz]

that did the trick, thanks!

[skibz]

@skellock i had a crack at making the transformers run serially - let me know if the approach i used is 👍 /👎 ?

(i'm not too happy with my solution, a higher-order promise function would be far nicer to read here. something like Promise.seq, if there were one.)

[craigcartmell]

How's this looking? :)

[skellock]

Haven't had the bandwidth just yet, sorry. It is a useful feature and I want this PR, however, I might tweak the implementation a bit. I have such a hard time wrapping my head around reduce for flow control. Or reduce in general for that matter. =)

[skibz]

yeah, the reduce usage is not pretty. 😔
i always thought it was weird that the standard doesn't include a function for serially processing a collection of promises.

[skellock]

Maybe a little help from this?

https://github.com/sindresorhus/p-waterfall

[skibz]

i think we have exorcised the ugly code demons

[skellock]

Sorry for the long cycle here. Thank you for contribution. I'll get a build out shortly with this.

[skibz]

thank you so much for letting me make this feature addition!