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

Allowing to pass options to Select.Async #1199

Closed
wants to merge 1 commit into from

Conversation

dstuecken
Copy link

@dstuecken dstuecken commented Sep 8, 2016

With this PR you are able to pass default options to Select.Async. This allows you to set a default value without calling the Async promise.

See Issue #1138

Example:

     const options = [{
        name: "My selected value",
        id: "1",
      }];

    const asyncOptions = {
      options,
      value: "1",
      // loadOptions: fetchFn,
    };

    return <Select.Async { ...asyncOptions } />;

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 91.643% when pulling 01a7bcc on dstuecken:master into fa569e1 on JedWatson:master.

@bvaughn
Copy link
Collaborator

bvaughn commented Sep 13, 2016

I'm concerned about the idea of supporting both options and loadOptions in Async. It seems ambiguous. (What's expected if both change? Which one overrides the other?) Also adding more forking logic seems undesirable.

Couldn't you achieve the same result by just returning default options the first time loadOptions was called? Something like...

let defaultOptions = [/*...*/]
let initialized = false

function loadOptions () {
  if (!initialized) {
    initialized = true
    return Promise.resolve(defaultOptions)
  } else {
    return fetch(/*...*/)
  }
}

@dstuecken
Copy link
Author

dstuecken commented Sep 13, 2016

You could, but i find it more confusing to have an options parameter that is omitted if you pass options to it and always do this kind of special handling you suggested.

Imagine a search form with over ten Async selects that you want to preselect. I wouldn't want to implement this 10 lines of code for each of them and handle initialization state for all of them.
I'd rather just want to set the selected option and give it a name without any workarounds and hassle - and not to forget, without fetching a range of data in 10 separate requests.

@bvaughn
Copy link
Collaborator

bvaughn commented Sep 13, 2016

You could, but i find it more confusing to have an options parameter that is omitted if you pass options to it and always do this kind of special handling you suggested.

That's totally fair. Your primary concern is maintainable application code and my primary concern (at least in this context) is maintainable library code. Those concerns won't always align. 😄

Imagine a search form with over ten Async selects that you want to preselect. I wouldn't want to implement this 10 lines of code for each of them and handle initialization state for all of them.
I'd rather just want to set the selected option and give it a name without any workarounds and hassle - and not to forget, without fetching a range of data in 10 separate requests.

This sounds like a situation where you could create your own decorator component or HOC to DRY up application code. For example:

import React, { Component, PropTypes } from 'react'
import { Async } from 'react-select'

class AsyncDecorator extends Component {
  static propTypes = {
    options: PropTypes.array.isRequired
  };

  loadOptionsWrapper (params) {
    const { loadOptions, options } = this.props

    if (!this.initialized) {
      this.initialized = true

      return Promise.resolve(options)
    } else {
      return loadOptions(params)
    }
  }

  render () {
    return (
      <Async
        {...this.props}
        loadOptions={::this.loadOptionsWrapper}
      />
    )
  }
}

@bvaughn
Copy link
Collaborator

bvaughn commented Sep 13, 2016

In general, I am wary of implementing more than 1 way to achieve the same result within library code because of the reasons I mentioned above:

  • When the 2 routes conflict, which one wins? (In many cases it's ambiguous and can be a source of bugs.)
  • Forks increase library maintenance load.

@dstuecken
Copy link
Author

dstuecken commented Sep 13, 2016

Yea sure, you can do many other things with HOC that are actually implemented in the library code, too :) I am not disagreeing with your points, they are all legitimate.
But don't you think it is more clear to have the same options parameter in either Select and Select.Async, especially new users will find that a hard nut to crack. I mean i was shocked at first time i realized options isn't doing anything in Select.Async :)
And this use case is not that special. Who wants a select that is not able to preselect "out-of-the-box"? :)

@bvaughn
Copy link
Collaborator

bvaughn commented Sep 13, 2016

I think your concerns are legitimate as well. 😄

I'll give this a little time to sink in and come back to it (unless Jed or another contributor beats me to it).

@bvaughn
Copy link
Collaborator

bvaughn commented Sep 13, 2016

Who wants a select that is not able to preselect "out-of-the-box"?

To be fair, this is only applicable to the special, async variation of Select. 😁

I'm not convinced that the Select.Async component is the simplest way of achieving async behavior. Seems to me like using a stateful/managed component that listens to onInputChange- refreshes its own options- and then re-renders with an updated options list passed to Select seems more straight forward. But I didn't write Select.Async and I haven't spent much time thinking about it so I might be overlooking something.

For example, something like:

class AsyncSelect extends Component {
  constructor (props, context) {
    super(props, context)

    this.state = {
      options: props.options || []
    }

    this._onInputChange = this._onInputChange.bind(this)
  }

  componentWillUpdate (nextProps, nextState) {
    if (this.props.options !== nextProps.options) {
      this.setState({
        options: nextProps.options
      })
    }
  }

  render () {
    return (
      <Select
        {...this.props}
        onInputChange={this._onInputChange}
        options={this.state.options}
      />
    );
  }

  _onInputChange (inputValue) {
    const promise = this.props.loadOptions(inputValue)

    this._promise = promise

    promise.then((options) => {
      // Ignore all but the most recent request
      if (promise === this._promise) {
        this.setState({ options })
      }
    })

    return inputValue
  }
}

Then again, I guess this isn't all that far off from what Select.Async is. Hm.

@bvaughn
Copy link
Collaborator

bvaughn commented Sep 13, 2016

Okay. Have thought through this a bit more now and have a better understanding about how Async works and why.

I believe I'll try my hand at refactoring Async. I'll come back to this PR afterward, but I'm pretty sure I can address the use-case you mentioned. :)

@bvaughn
Copy link
Collaborator

bvaughn commented Sep 18, 2016

Closing in favor of PR #1226. I refactored Async and resolved a few other related issues along the way. Thanks again for the contribution and discussion!

@bvaughn bvaughn closed this Sep 18, 2016
@bvaughn bvaughn mentioned this pull request Sep 18, 2016
@dstuecken
Copy link
Author

Hey Brian,

How did you resolve this issue btw?

@bvaughn
Copy link
Collaborator

bvaughn commented Mar 29, 2017

#1226

@dstuecken
Copy link
Author

Ok checked the code, passing options seems to work now with Select.Async 👍 Thanks

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 this pull request may close these issues.

3 participants