Creatable and Async components can compose each other

[bvaughn]

Added child-function support to Async and Creatable components so that they can compose each other (or future HOCs).

While this PR adds support for composing- unfortunately the 2 don't seem to play nicely together as can be seen by applying the following diff and playing with the "Github users" example.

diff --git a/examples/src/components/GithubUsers.js b/examples/src/components/GithubUsers.js
index a206477..b778a21 100644
--- a/examples/src/components/GithubUsers.js
+++ b/examples/src/components/GithubUsers.js
@@ -44,7 +44,9 @@ const GithubUsers = React.createClass({
        return (
            <div className="section">
                <h3 className="section-heading">{this.props.label}</h3>
-               <Select.Async multi={this.state.multi} value={this.state.value} onChange={this.onChange} onValueClick={this.gotoUser} valueKey="id" labelKey="login" loadOptions={this.getUsers} minimumInput={1} backspaceRemoves={false} />
+               <Select.Async multi={this.state.multi} value={this.state.value} onChange={this.onChange} onValueClick={this.gotoUser} valueKey="id" labelKey="login" loadOptions={this.getUsers} minimumInput={1} backspaceRemoves={false}>
+                   {(props) => <Select.Creatable {...props} />}
+               </Select.Async>
                <div className="checkbox-list">
                    <label className="checkbox">
                        <input type="radio" className="checkbox-control" checked={this.state.multi} onChange={this.switchToMulti}/>

Resolves issue #1200

[coveralls]

Coverage increased (+0.3%) to 92.179% when pulling a863d15 on 1200 into fa569e1 on master.

[JedWatson]

Nice, looks good! Thanks @bvaughn

[bvaughn]

Hey @JedWatson,

I got a few more minutes to think about this this afternoon and I'm no longer sure that arbitrary HOC composition is doable (at least not easily) with 1.x.

For starters, there are several places in Creatable (and one in Async) where the components expect to a this.select ref to point to an instance of Select. That can be worked around by this (kind of nasty) patch:

diff --git a/src/Async.js b/src/Async.js
index 6840fc3..15e26de 100644
--- a/src/Async.js
+++ b/src/Async.js
@@ -89,7 +89,7 @@ const Async = React.createClass({
        }
    },
    focus () {
-       this.select.focus();
+       this.getInnerSelect().focus();
    },
    resetState () {
        this._currentRequestId = -1;
@@ -98,6 +98,17 @@ const Async = React.createClass({
            options: [],
        });
    },
+   getInnerSelect () {
+       let select = this.select
+
+       while (select) {
+           if (select instanceof Select) {
+               return select
+           } else {
+               select = select.select
+           }
+       }
+   },
    getResponseHandler (input) {
        let _requestId = this._currentRequestId = requestId++;
        return (err, data) => {
diff --git a/src/Creatable.js b/src/Creatable.js
index bdaadf3..4d9053d 100644
--- a/src/Creatable.js
+++ b/src/Creatable.js
@@ -62,9 +62,12 @@ const Creatable = React.createClass({

    createNewOption () {
        const { isValidNewOption, newOptionCreator, shouldKeyDownEventCreateNewOption } = this.props;
-       const { labelKey, options, valueKey } = this.select.props;

-       const inputValue = this.select.getInputValue();
+       const select = this.getInnerSelect()
+
+       const { labelKey, options, valueKey } = select.props;
+
+       const inputValue = select.getInputValue()

        if (isValidNewOption({ label: inputValue })) {
            const option = newOptionCreator({ label: inputValue, labelKey, valueKey });
@@ -74,7 +77,7 @@ const Creatable = React.createClass({
            if (isOptionUnique) {
                options.unshift(option);

-               this.select.selectValue(option);
+               select.selectValue(option);
            }
        }
    },
@@ -84,13 +87,14 @@ const Creatable = React.createClass({

        const filteredOptions = filterOptions(...params);

-       const inputValue = this.select
-           ? this.select.getInputValue()
+       const select = this.getInnerSelect()
+       const inputValue = select
+           ? select.getInputValue()
            : '';

        if (isValidNewOption({ label: inputValue })) {
            const { newOptionCreator } = this.props;
-           const { labelKey, options, valueKey } = this.select.props;
+           const { labelKey, options, valueKey } = select.props;

            const option = newOptionCreator({ label: inputValue, labelKey, valueKey });

@@ -110,18 +114,31 @@ const Creatable = React.createClass({
        return filteredOptions;
    },

+   getInnerSelect () {
+       let select = this.select
+
+       while (select) {
+           if (select instanceof Select) {
+               return select
+           } else {
+               select = select.select
+           }
+       }
+   },
+
    isOptionUnique ({
        option,
        options
    }) {
-       if (!this.select) {
+       const select = this.getInnerSelect()
+       if (!select) {
            return false;
        }

        const { isOptionUnique } = this.props;
-       const { labelKey, valueKey } = this.select.props;
+       const { labelKey, valueKey } = select.props;

-       options = options || this.select.filterOptions();
+       options = options || select.filterOptions();

        return isOptionUnique({
            labelKey,
@@ -142,7 +159,8 @@ const Creatable = React.createClass({

    onInputKeyDown (event) {
        const { shouldKeyDownEventCreateNewOption } = this.props;
-       const focusedOption = this.select.getFocusedOption();
+       const select = this.getInnerSelect()
+       const focusedOption = select.getFocusedOption();

        if (
            focusedOption &&
@@ -160,7 +178,7 @@ const Creatable = React.createClass({
        if (option === this._createPlaceholderOption) {
            this.createNewOption();
        } else {
-           this.select.selectValue(option);
+           select.selectValue(option);
        }
    },

After this change though, Async's async options are still clobbering Creatable's prepending of the special "create option..." placeholder (which means the option-creation doesn't really work).

I think we should consider reverting this PR for now.

[bvaughn]

Scratch the awkward diff above. We could avoid it by nesting HOCs like so:

<Async {...props}>
  {(asyncProps) => (
    <Creatable>
      {(creatableProps) => (
        <Select
          {...asyncProps}
          {...creatableProps}
          ref={(ref) => {
            // This is obviously a bit hacky ;)
            asyncProps.ref(ref)
            creatableProps.ref(ref)
          }}
        />
      )}
    </Creatable>
  )}
</Async>

Unfortunately the options-clobbering is still an issue. Hmm...

[JedWatson]

Maybe we could do this better by creating a AsyncCreatable HOC that merges logic from Async and Creatable?

I haven't had a chance to look at what that would look like, not sure if we could do it neatly or if it would just lead to a bunch of code duplication. Depends on how cleanly the functions overlap, I think?

[bvaughn]

Maybe. That feels like a sad path to start down though because it doesn't scale well. 😞

I'll poke at it some more. Maybe there's still a less extreme option.

[bvaughn]

I think a fundamental problem may exist with these lines. They are clearing the input value as a result of Async.onInputChange which is in turn breaking Creatable (since it thinks there's no input text).

Not yet sure why the state is being cleared. Still tracing through...

Edit: That's only part of the problem. Creatable needs to tap into componentWillUpdate in order to be notified of externally updated options (eg when Async loads new data) but at the point when CWU executes- the ref has been cleared out. Maybe I need to switch Creatable over to sync options between props and state...

[bvaughn]

Progress!

[bvaughn]

Let's continue discussion over on PR #1210 (demo and updated code)