Select helper fixes

[jpuri]

Hey @nikgraf ,

This PR has changes in helper methods. If I pass a non-iterable object to any helper method expecting an array, it should treat it like array of single object. Its different from underscore which typically in such cases iterate over each field of object.

The reason for doing this change is select's typical case:

<select>
  <option>abc</option>
</select>

When select has single option react does not makes and array but single object which was breaking when we passed it to helper.filter. I did change in all methods as I wanted behavior to be consistent.

Changes look good now, can be merged.

[nikgraf]

Multiple left-hand assignments have the strange side-effect in JS that they are then in the global scope. From what I learned it's best to avoid them at all. see http://stackoverflow.com/a/1758912/837709

[jpuri]

Agreed, I was not sure of this change. Just got motivated from underscore's code, They used it so much. Let me revert this :)

[nikgraf]

@jpuri I'm bit worried about updating all the helpers. There are a lot of things that can go wrong in various browsers and underscore is heavily used & tested. Also the further we move away from underscore the harder it will be to apply bug-fixes they do.

Would it have been possible to just fix the whole issue by doing this?

if (this.props.children instanceof Array) {
  this.sanitizedChildren = this.props.children;
} else {
  this.sanitizedChildren = [this.props.children];
}

If we then always use this.sanitizedChildren we should be pretty much save?

[jpuri]

Hey Nik,

My implementation of helpers is not underscore, underscore takes care of a hell lot of edge cases and ECMA versions. I want to keep implementation minimal and tailored specifically to our need.

It definitely can be handled the way you are mentioning. But if we change helpers it will be change in one centralized place (it will not be ideal to need to do this for all props of type array :( ). I want to have clear expectation set from helper methods and all helper methods should behave in that way.

By moving away from underscore we definitely are losing advantage of battle tested code. But reverse engineering a helpers for us from underscore is crazy plus we might still be left with unexpected behaviors and doing some of the handling in components code itself.

[jpuri]

My brain is kind of loaded with thoughts about what would be the best way out for this. I do not want to have some black box code in my app which I just trust.

The reason <select> was not breaking previously was that when we passed single object to it underscore was doing filtering on each field of the object and returning empty object. This was not what we needed but we never knew - some edge case might not have worked well, but we never noticed.

So there are trade-offs, loosing the advantage of some battle tested code (which is no small in itself) vs loosing the advantage of having a helper perfectly suited to what I need (this will need that we have exact knowledge of underscore method when we use it and we tailor our input to the expectations of the method).

Lets discuss this and the other PR more in call today.

[jpuri]

Hey Nik,

I have updated the PR, but still there is one issue - if there is no placeholder(which is not highly probable) and no option then select gets broken on the page as <div> for placeholder or option is missing.

[nikgraf]

Hey@jpuri - I finally found some time to review and work on this PR in detail.

I looked at it not only from the perspective of providing one single option, but also combining a single placeholder with an array of options (see #161). Rather than cover all possible edge cases in the helpers I propose we move towards sanitising the props.children before using them.
This can simply be done by using flatten. I tested my assumptions by updating this PR, but we can always go back.

Let me know what you think.

[jpuri]

Hey @nikgraf ,

Changes in select look great 👍 (I will spend some time testing those today).

I agree with you about helpers we should use more of native methods. There are many in ES5 which can greatly help us (I will create a issue for this).
I want helpers to have consistent and well defined behavior (more tailored to what belle code needs), so that when we use it we know what exactly we are doing - that also makes me happy about getting rid of underscore.

I still want to keep mapObject method because otherwise map method handling both array and object becomes inconsistent from what other methods do. Let me add a commit for that.

[nikgraf]

@jpuri sounds good

👍 on mapObject

[jpuri]

Hey Nik,

In case select has no children. its hight become 0. Which looks bad, this can be fixed by giving a min-height.
What would you suggest for this.

I am yet to do that mapObject changes - planning to do it tomorrow.

[nikgraf]

@jpuri no children also bugged me when working on this. I figured a Select without Options or a Placeholder doesn't make sense to me and it should be mandatory.

Thoughts?

[jpuri]

Yes, its very unlikely to have such scenario (I think of scenario where options are dynamically and there is no option in some scenario).

Lets leave it as is :)