Accept promises in asyncOptions alongside callbacks

[dmatteo]

This PR is addressing the inability of asyncOptions to handle promises, as discussed in #491

I'm keeping commits separated by concerns (component, searchingText tests, searchPrompt tests) for easy review.

Before moving on to the rest of the failing tests, I would like to hear what you think about asyncOptions only being able to handle promises from now on.

[brianreavis]

I don't think it's a good idea to decide for people that they'll need to bring in a hefty promises library just to use react-select (especially just for one feature). Supporting promises does not have to equal requiring promises. Why not just test for the presence of .then on what's returned by asyncOptions?

[dmatteo]

They don't need any promise library if they don't use asyncOptions.
If they need it though, I don't see how a delayed callback could be of any use in a real use case.

You probably fetch your asynchronous options from some endpoint, using http calls that returns promises (unless they're blocking).

We could easily test for .then but it just seems to bloat the code to me.

[brianreavis]

If they need it though, I don't see how a delayed callback could be of any use in a real use case.

It's working for plenty of people as it is, no?

You probably fetch your asynchronous options from some endpoint, using http calls that returns promises (unless they're blocking).

XMLHttpRequest at its lowest level uses callbacks and has no notion of promises. There are plenty of lightweight ajax libraries that don't include promises, like most in the list here: http://microjs.com/#ajax.

All in all, I'm not sure if I get the argument here. We can't tell people not using a huge ajax implementation that they're out of luck because we assume everyone is using promises. If added complexity is the worry and the .then check is too much, let's just keep callbacks. It's not broken and doesn't make any assumptions for the user.

// what's proposed
return fetch(`some/uri/with/${query}`)
   .then((response) => {
     return response.json();
   });

// all that's needed w/o promise support
return fetch(`some/uri/with/${query}`)
   .then((response) => {
     callback(null, response.json());
   })

[JedWatson]

I'm happy to add Promise support, given the upcoming Fetch API works with them and it's a popular pattern, but not at the cost of callback support.

For one, this would be a major and unnecessary (imo) breaking change for a lot of current users; it also, as @brianreavis mentions, means that - for a while at least - including a Promise library would be necessary to use asyncOptions.

I was thinking about whether we should support a different prop for the Promise asyncOption but I'm concerned that's too obscure...

Wouldn't this do the trick?

let callback = (err, options) => { /* ... */ }
let asyncOptions = this.props.asyncOptions(input, callback);
if (asyncOptions && asyncOptions.then) {
    asyncOptions.then(options => callback(null, options)).catch(err => callback(err));
}

[dmatteo]

Fair enough @brianreavis, you do have a point.
People probably found a way to use it as it is, although your example feel to me like a workaround more than a proper usage.

@JedWatson something like that could work, but we also need to provide a way to cache the results with cacheAsyncResults.

I'll update the PR later today, cheers

[JedWatson]

Thanks @dmatteo 👍

[dmatteo]

Ok, I really need some help here, as I don't know exactly how to proceed.

I have got asyncOptions to accept a function that takes an input (the query) and returns a promise, and that is working fine with caching and all.

I have a working example here: https://github.com/dmatteo/react-select/tree/asyncExamples.
When you search for ch you will wait 2 seconds the first time, and after that it will be istant. Then again, if you search for cho you will wait another 2 seconds, and this is how caching is supposed to work, if I got it right.

However, some tests are failing on the caching, as apparently the function gets called more than necessary.
Can you guys see something I did wrong or should the test be adjusted?

p.s. @JedWatson you sure we don't want a new prop? :)

[brianreavis]

Do you mean to be calling asyncOptions() three times like this? If you follow @JedWatson's example, the behavior of react-select with and without promises shouldn't be any different.

[dmatteo]

I would love not to call it three times like this, as I agree it's pretty bad.

Unfortunately @JedWatson approach is not enough here, since asyncOptions it's a function that returns a promise, not a promise itself, in order to be able to take an input.

How do you check if a function returns a promise without calling the function itself?

[JedWatson]

You call this.props.asyncOptions(input) and assign the result to a variable. Then check the variable instead of subsequent calls to this.props.asyncOptions().

Basically:

        let result = this.props.asyncOptions(input);
        if (result !== 'undefined' &&
            typeof result.then === 'function') {
            result.then((data) => {

[JedWatson]

But to be clear, you need to also support the callback case here, so be sure to pass the callback argument to asyncOptions when you invoke it:

let result = this.props.asyncOptions(input, callback);

(haven't looked at how else you'd structure your code to allow this, I'm out and about at the moment but if you need more help I'll check back later)

[dmatteo]

You call this.props.asyncOptions(input) and assign the result to a variable.

Meh, of course 😴

But to be clear, you need to also support the callback case here,

Yep, I am calling it in https://github.com/dmatteo/react-select/blob/asyncOptions/src/Select.js#L594-L596, but I'm not passing it in the promise.

[dmatteo]

Ok, the save the return value inspired me an even better idea.

I'm saving the return value of the first asyncOptions call (the callback one) and after that has being executed I check if the result is a promise and then execute the promise logic.
No double executions of asyncOptions in any case.

The only downside I see is that if users provide a function that returns a promise and also accepts a callback with 2 params, it will run both the callback and the promise paths.

What do you think?

[dmatteo]

As soon as I have positive feedback I'll write a few tests for this.

[JedWatson]

@dmatteo looks good to me.

To challenge you though: how about we reduce code duplication & abstract the handling of promise return vs. the callback?

If you define the callback before assigning asyncOpts, you can reuse it in the Promise:

let callback = (err, data) => { /* ... */ };
let asyncOpts = this.props.asyncOptions(input, callback);
if (asyncOpts && typeof asyncOpts.then === 'function') {
  asyncOpts.then(data => callback(null, data)).catch(err => callback(err));
}

[JedWatson]

You're re-calling this.props.asyncOptions(input) rather than just using asyncOpts.then

[dmatteo]

oops, nice catch!

[dmatteo]

@JedWatson I overlooked the fact that the callback was exactly the same, thanks!
Much more dry.

I'll add some tests now

[dmatteo]

@JedWatson I've added some tests (more to come) and I'm using an internal var to keep track of the number of calls and the input that is passed in (dmatteo@4d1673e#diff-f227bbc93e56e86e165bcdf5e07e8ea4R1389)

Since I'm not using spies there, it seemed like a good way to go.

What do you think?

[JedWatson]

@dmatteo looks good to me, I'm going to defer to our test master @bruderstein though in case he's got any feedback on that :)

[bruderstein]

I definitely can't live up to that title 😄

Is there any reason why you're not using spies there? That may make it easier and more readable (including the assertions) - just wrap your function with sinon.spy(...)

asyncOptions = sinon.spy((input) => {
              const options = [
                    { value: 'test', label: 'TEST one' },
                    { value: 'test2', label: 'TEST two' },
                    { value: 'tell', label: 'TELL three' }
                ].filter((elm) => {
                  return (elm.value.indexOf(input) !== -1 || elm.label.indexOf(input) !== -1);
                });

                return new Promise((resolve) => {
                    resolve({options: options});
                })
            }
);

Then your test for calling etc can use the built in assertions

it('should be called once at the beginning', () => {
                expect(asyncOptions, 'was called once');
});

(You get a nicer error message if it fails too)

The three tests that test your asyncOptions mock, I'd personally put in a nested describe, to identify explicitly that they're only testing the mock, and not meant to be exercising any non-test code. Something like

describe('[mocked asyncOptions]', () => {
       it('should fulfill asyncOptions promise', () => {
             ...
    });

    it('should fulfill with 3 options when asyncOptions promise when input = "te"', () => {
             ...
    });

    it('should fulfill with 2 options when asyncOptions promise when input = "tes"', () => {
            ...
    });
});

This is just a style thing though, it just took me a moment to realise what the tests were trying to prove.

Then the last test that tests calling the asyncOptions (calls the asyncOptions again when the input changes), can use the built in assertions due to the spy

it('calls the asyncOptions again when the input changes', () => {
                typeSearchText('ab');
                expect(asyncOptions, 'was called with', 'ab');
                expect(asyncOptions, 'was called times', 2);
});

You get a much nicer error message if it fails here too, especially if it wasn't called with the right arguments, you get to see what it was called with.

I'd actually add two further tests here, and one of them is probably the most important for this change, in that we need to check that it actually updates the options with what the asyncOptions returns in the promise (ie. calls callbackFoo). This is a bit of a giggle due to the enforced async nature of the promise, but I think we can probably do it reasonably nicely.

If you add another .then(...) to the returned promise, it should run (hopefully) after the then() that has already been added in Select.js (I'm not sure if this is part of the spec for promises, or even if it works like that, I'm just kinda hopin'!).

In theory, you can grab the promise from the return value in the spy, and then just add your then() with your assertions (I've not tried this out, so apologies if there are errors here)

it('updates the options after the promise returns', () => {
    typeSearchText('te');
    return asyncOptions.firstCall.returnValue.then(() => {
        expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option',
                    'to satisfy', [
                        expect.it('to have text', 'TEST one'),
                        expect.it('to have text', 'TEST two'),
                        expect.it('to have text', 'TELL three')
                    ]);
        });
});

The second test I'd add, would be one where the promise rejects. I'm not quite sure what to expect if that happens, I guess we'd just check that the previous options weren't changed? So maybe return a resolved promise on the first call, then a rejected on the second, and check the options are still the same as after the first promise resolved.

Sorry for the long comment - hope it's helpful, just my 2 cents.

[dmatteo]

Hey @bruderstein, thanks a lot for your very thorough comment!

Is there any reason why you're not using spies there? That may make it easier and more readable (including the assertions) - just wrap your function with sinon.spy(...)

No reason if not my inability to make it work with spies. I had tried different approaches, and none of them worked, but again, I didn't know that you could wrap a function in a spy like that.
On the bright side, I've learned something new :)

The three tests that test your asyncOptions mock, I'd personally put in a nested describe, to identify explicitly that they're only testing the mock, and not meant to be exercising any non-test code. Something like

Makes sense, I'll do that before the "actual" tests

As for testing the outcome of the resolved promise in the DOM, that's was what I still had to wrap my head around.
Your suggestion could work, I will give it a try asap. If anything, it's a great starting point.

Thanks again and I'll ping you back when I have something

Cheers

[JedWatson]

Thanks @bruderstein! Great write-up.

@vitalbone lots of good info here you may be interested in too re: writing tests for Keystone

[dmatteo]

Ofc, I'll squash and rebase appropriately when we're done 😄

[dmatteo]

ping @JedWatson @bruderstein

[bruderstein]

Sorry, been busy with other stuff :(

Looks good - I had a thought one night but not had time to have a proper look, that maybe we could simplify the mock with a stub that just returns a fixed promise based on the value, but it's up to you.

const asyncOptions = sinon.stub();
asyncOptions.withArgs('').returns(Promise.resolve({ options: [ /* ... default options */]}))
asyncOptions.withArgs('te').returns(Promise.resolve({ options: [ /* ... te options */] }));
asyncOptions.withArgs('tes').returns(Promise.resolve({ options: [ /*... tes options */] }));
asyncOptions.withArgs('fail').returns(Promise.reject(new Error('something is wrong, jim')));

Which would save the tests on the mock, and actually having any logic in the mock.

I've not tried this out - not sure it's "better", but either way, the tests now are looking good!

Sorry for the delay in feedback :(

[dmatteo]

No worries for the delay :)

I'm not entirely sure that the "full stubs" version is better than the mocked promise though.
True, you have 3 extra tests for the mock, but they're not very heavy.
On the other hand, I think the "mock logic" is more readable and understandable, thus maintainable.

What do you think?

[dmatteo]

@bruderstein reping, sorry :)

[bruderstein]

I'll leave this to you - both are reasonable approaches, and both are pretty understandable.

[dmatteo]

Alrighty, I'd rather keep it like this then.

@JedWatson I've squashed and rebased, we should be ready to go!

[dmatteo]

@JedWatson ping?

[JedWatson]

Sorry @dmatteo! missed the notification when you wrapped this up. Good to go. It's nearly 2am here so I'll publish tomorrow when I'm awake :)

[JedWatson]

and thank you for your work on this!

[dmatteo]

Eheh no worries.
Hope I didn't wake you up with the notification :)

Thanks to you for the great component!