Potential Refactor

[brianreavis]

First, react-select is slick! It's been awesome to work with. I've been wanting to make some bugfixes, but have ended up with my hands tied because of a few things that are hard to reason about. This list also contains some other notes:

• Drop the "dist" files altogether. React's already basically impossible to use without a bundler, so I'm not sure if it's doing much good? "Please don't update the dist files" gets really old to say to the million people who don't read the contributing notes.
• Build "lib" when publishing, but ignore in repo (same point as above)
• Reduce/reconsider usage of componentWillMount? It's triggered on the server, so calling autoloadAsyncOptions and such here seems strange. Also, I'm not sure this._bindCloseMenuIfClickedOutside = ... etc are assigned in this way here?
• Enforce "options" prop to be immutable. Whenever a prop changes, there's two JSON.stringify calls in componentWillReceiveProps that are expensive for large datasets. It'd be more efficient if there could just be a strict equality check here.
• Move functionality not directly related to the control out to separate files/modules. The includes:
  • DOM event binding
  • Option searching (for selectize I split this out to sifter.js because it ballooned and needlessly complicated the library)
• Consolidate menu opening code to happen in one spot. As it is right now, the menu's events are bound multiple places throughout the library. I think the intent is to consolidate setState calls? React will batch these so I think's a bit moot and leading to more problems than it's solving? It's tough to track down where events are bound/unbound.
• Don't modify state directly (esp in render methods). e.g. L630-L634
• Use ...spread in cases like these.
• Why does the this context matter on callbacks? E.g. callback.call(this, {}); All React element methods are auto-bound already.
• Consider Allow any type in value #25. Given that React's point is abstracting away DOM craziness, I think that the "values must be strings" is quite limiting... it leads to lots of application-level casting in onChange events. It feels very weird to cast an ID to a string for <Select> then cast it back to a number.

@JedWatson @dcousens What are your thoughts on these? I almost started on a lot of these today, but it'll create a lot of merge conflicts on existing PRs and be a little risky w/o tests.

[JedWatson]

Hey @brianreavis, good to have you here!

Would be really happy to have your help addressing these. I agree with most of what you've listed, and have been meaning to do a fairly large refactor for a while (especially the "values must be strings" limitation).

Specifics:

• dist files, I'm happy to look at removing these, they were added specifically because of requests for publishing to bower though. Maybe we should replace it with instructions on how to build them yourself if you want them (I'm personally not a fan of Bower... and agree that React is basically impossible to use without a bundler, but the requests happened so I went with it).
• Would love to ignore lib in the repo, for the same reason, however bower (and anyone who's not using npm) download directly from github, which makes it hard to exclude. Perhaps the real answer is just to provide a really easy-to-follow process to generate this stuff yourself post-download. We should discuss this in a standalone issue and get some feedback I think.
• Immutable options sounds like a good solution
• So does restructuring, would be good (I think) to bundle something simple and allow sifter (or similar) to be plugged in, love the work you did with that
• The events are set up in a pretty tricky way at the moment because of hoops I had to jump through to get the interaction model(s) working properly. This was my first go at a complex React UI component (it's had one major refactor, but that was a while ago) and everything was done for a reason, but I expect we can improve it considerably. Let's see how it goes.
• +1 for not modifying state directly - shouldn't be done like that
• +1 for spread
• Callback scope is set because _bindCloseMenuIfClickedOutside is created when the component mounts, and not as a normal class method (so it's not auto-bound)

And finally - I'm really interested in allowing options and values to be provided as any kind of object. The original use-case for this was a "drop-in" replacement for something like Selectize, in a React app. Which works great if you're dealing with forms, but in a more sophisticated SPA this really holds it back.

We should also make it easier (without hacks) to replace any child component (menu items, value renderer, etc) with a custom component that would take the value / option data and render it however it likes.

Let's clean up the current PRs as much as we can then start prepping for a new refactored version. If we can get these issues (particularly configurable components and complex values) in place that's probably a good target for releasing v1.0.0 which would be fine to include breaking changes as necessary.

[AlexKVal]

As a solution for Bower distr:

• create standalone git repository for exactly Bower
• push by releasing tools to it those autogenerated dist and lib

[dcousens]

@JedWatson I concur with @AlexKVal with making a separate repository just for the dist mess that is Bower.

@brianreavis these suggestions are awesome.

[dcousens]

@brianreavis any ideas on how we could avoid a lib/ completely?

I understand that would just mean we push the transpilation steps higher up the dependency chain, which is fine, but experience has told me that can be a real bugger for those using web pack; so that is why the current solution is holding its own.
What are your thoughts?

[brianreavis]

@dcousens Hmm, I'm not sure we can drop it altogether. In webpack guides it's common for the jsx-loader/babel-loader to explicitly ignore the node_modules folder – which would prevent us from letting it take care of the transpilation for us.

I think the best plan of attack is:

• Delete "lib" directory and push
• Add "lib" to .gitgnore
• Add an .npmignore file that's identical to .gitignore, except it should exclude the line that causes "lib" to be ignored.
• Build lib as part of the "prepublish" step
• Delete the directory in "postpublish" (optional)

What do you think?

[AlexKVal]

.npmignore is blacklisting.
I propose to use whitelisting instead by adding into
package.json

"files": [
  "LICENSE",
  "README.md",
  "CHANGELOG.md",
  "lib",
  "dist",
  "... whatever ... "
]

[brianreavis]

👍 Works for me

[doctorallen]

Additionally, when you create a release on GitHub, you can upload binaries if you want, so you can include the already compiled css and js in a zip or tar for each release, and keep it out of the Git repo.

[JedWatson]

Closing in favour of #568

[JedWatson]

Just for fun I've ticked off the concerns that are addressed in the rewrite.

Those that aren't:

• bundling and dist builds are tricky, how it's currently set up is kind of the "lesser evil" at this point
• Some concerns have been separated but currently all the methods that are still in the component itself (e.g. filtering) make heavy use of props and/or state, so it's hard to split them out more at this stage. something to review down the track.