Add multiple selection feature

[miguelcobain]

I think I nailed it! Example in the dummy app.
I'm not very familiar with mocha. Maybe you could write the needed tests.
Otherwise I can certainly look into it later.

This PR includes #6.

[miguelcobain]

This replaces the array every time a change happens.
Don't know if it causes any problems.

We could use something like setObjects.
Even better, if we could know which option was selected/deselected we could add/remove to the array and trigger some new add/remove actions.

AFAIK there isn't any way to know which option was selected/deselected in a change event, right?

[cowboyd]

I don't believe there is, although it might be worth investigating if there is relatedTarget which points to the option in the change event. Even if it doesn't we can do internal book keeping to raise "on-add" and "on-remove" actions as a separate PR.

Since the philosophy of this component is to hue as closely to "what would html do", I think that the answer is to completely replace the array, and raise the action with the new array as its argument.

This is how multiselect works on native selects and file inputs, so it should be the default. That said, there are going to be people who want to bind their ember-data relationships directly to an x-select and so it might make sense to add an in-place-edit=true option that uses setObjects, but that can come later, if at all.

In any case, it looks like the native <select> element actually separates out its value and selectedOptions into two separate attributes, where value is always the first selected option, whether the select is multiple or not, so maybe we should do the same. In the case of multiple, you would want to bind selectedOptions. That way you don't have value sometimes as an array and sometimes a scalar.

The action then, would always pass the value and selectedOptions.

https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement

[miguelcobain]

This is obviously your call, but I'll leave here my point of view on this.

I think that, more importantly than what HTML does, is what Ember does.
As both an Ember user and maintainer of ember-cli-selectize: people will want to use this with ember-data. And I believe this is the whole point in all Ember things. We should always try to guess what the developer will do instead of expecting them to think in any other mindset (like HTML). Another way to say it is that I think we should make the biggest number of developers happy.

That being said:

• I think that value being an array only when multiple=true is totally fine and expected. I mean, what would a developer could think otherwise? If they're saying that multiple=true they can only expect multiple values, i.e an array. Also, there is documentation to help.
• I think that an in-place approach should be the default and the only approach. It clearly makes the majority of the users happy. Besides that, it's the least intrusive way of doing things: if the user gives us an array, why should we replace it with a new one? It may lead to unexpected outcomes, in my opinion.
• With this PR, action is already triggered with the current array, I didn't understand if that was clear to you.
• With Ember supporting more and more "data down, actions up", I think that add and remove actions with the added and removed items as parameters respectively would be very nice to have.

I think of this addon as the way to use native selects in Ember. Maybe we could even have this to replace the current Ember.Select or inspire someone who does its refactor.
If users want a more sophisticated alternative they can use ember-cli-selectize or other wrappers. I ended up needing this for use cases where I need native selects (mobile browsers handle them really well).

I also think that we should have both a block and blockless version. The blockless version would provide an API similar to Ember.Select and ember-cli-selectize:

{{x-select content=items
                value=model.item
                optionLabelPath="content.name" }}

This way I think we would cover all the use cases a user might want for a native select.
From a simple harcoded select:

{{#x-select value=bob action="selectPerson"}}
  {{#x-option value=fred}}Fred Flintstone{{/x-option}}
  {{#x-option value=bob}}Bob Newhart{{/x-option}}
{{/x-select}}

To a full blown multiple select with actions

{{x-select content=items
                value=model.item
                optionLabelPath="content.name"
                multiple=true
                disabled=isDisabled
                ... }}

If our POVs are in sync, I can work on making this PR reflect what I said here.

[cowboyd]

You make a convincing case.

I think we can agree on the principle that we want to do what surprises the developer least. In my case, I have always found the ember select view interface confusing and brittle, and so we wrote this component because it aligns very cleanly with the resulting HTML... what you see is what you get. None of the things you propose conflict with that in principle, I just want to make sure that we don't lose sight of that happy path with any addittions to the feature set.

I think that value being an array only when multiple=true is totally fine and expected. I mean, what would a developer could think otherwise? If they're saying that multiple=true they can only expect multiple values, i.e an array. Also, there is documentation to help.

I am ok with overloading the value attribute to contain both an array and a scalar depending on whether the multipleattribute is set, especially since the binding case can get really confusing if both value and selectedOptions attributes are considered live and two-way bound.

I think that an in-place approach should be the default and the only approach. It clearly makes the majority of the users happy. Besides that, it's the least intrusive way of doing things: if the user gives us an array, why should we replace it with a new one? It may lead to unexpected outcomes, in my opinion.

What I meant to say was that the array that is passed in be treated as immutable and left untouched by default. Then, a new array, containing a snapshot of the fully realized changes is passed every time in the action. To me, this seems the most compatible with "data down, actions up". The in-place option would permit the component to mutate the array... basically equivalent to value={{mut anArray}} in 2.0

Mutating the array in place also gives me pause from a practical standpoint. I don't think I've ever not regretted editing an ember-data hasMany relationship in-place. Because they don't affect record dirty tracking, your store can get into an inconsistent state without any way to rollback. There are also dragons behind passing in a computed array that can be very confusing to new-comers. Because of the pain that I've personally felt on these issues, I'm hesitant to recommend it to other users by way of the default.

I'm going to inquire further about what immutability by default actually means in 2.0 means w.r.t. arrays.

Thoughts?

With Ember supporting more and more "data down, actions up", I think that add and remove actions with the added and removed items as parameters respectively would be very nice to have.

I agree whole-heartedly, just think it might make sense to put it in as a separate PR.

I also think that we should have both a block and blockless version. The blockless version would provide an API similar to Ember.Select and ember-cli-selectize:

I was initially opposed to this for the reasons I stated above. However, we recently converted a project over from using ember-select-2 to your ember-cli-selectize addon. The fact that they both kept faith with the original ember select view's api made that migration a breeze. I'll open up an issue to track that feature at #8 .

[miguelcobain]

Regarding mutability of the value array, I think that if someone gives us value=model.items, he/she implicitly intend us to mutate that array, right? If he/she wants to mutate the array himself through actions, he wouldn't even write value=model.items. Maybe I'm missing something?

Also, "data down, actions up" is not something "written in stone". Please see my comment here miguelcobain/ember-cli-selectize#34 (comment)
When we reach Ember 2.0 era, we may still write a lot of value={{mut anArray}}.

So, what's missing in this PR? Actions?

[cowboyd]

Regarding mutability of the value array, I think that if someone gives us value=model.items, he/she implicitly intend us to mutate that array, right? If he/she wants to mutate the array himself through actions, he wouldn't even write value=model.items. Maybe I'm missing something?

There are, I think, good arguments for why you would not want that by default, and it basically follows the same logic as those put forth in the 2.0 RFC. It boils down to the fact that if you are changing the state of your routes's models outside the routes themselves, you run a far greater risk of introducing non-determinism into your application. That's why mutability is becoming a simple, yet explicit opt-in.

Something I'd forgotten though: 2.0 is not here yet, and as you point out, it is not set in stone, and is subject to change. The current expectation under 1.x rules is that all bindings are both ways by default. This component in it's 1.x incarnation should abide by 1.x rules, which are well-known. We can cross the 2.0 bridge when we come to it.

As an aside: When handling form input, we've found that the best way to maintain sanity is to make a copy of our model into memory as a simple Ember.Object that can be edited in place and can be thrown away (on a click of "cancel" for example) without any consequences. We copy the model so that we are free to mutate the array inline, although it's worth noting that all of the changes to the original model get applied in an action (while we don't use the BufferedProxy pattern you see often in the wild, it's basically the same idea). Personally, for us, having it in-place would be the most common usage, but we jump through some extra hoops to make that safe. So yes, in the 2.0 era, we'll have lots of {{mut anArray}} in our code 😧

So, what's missing in this PR? Actions?

• Tests. You'd asked for some help writing those, which I can certainly do.
• Can you rebase onto master which includes the upgrade to ember 1.10 / ember-cli 0.2.0
• Should it be a hard error if the select box is multiple and the bound value is non-null and not an array? My inclination is yes.

[miguelcobain]

Notice that if the user never writes value=model.whatever we will never change any data at all! Isn't this "Ember 2.0" compliant? I don't know what work will be needed to become more compliant to "Data down, actions up" than this.

Can you rebase onto master which includes the upgrade to ember 1.10 / ember-cli 0.2.0

I think this PR included that other PR. I started from that point. Is that enough?

Should it be a hard error if the select box is multiple and the bound value is non-null and not an array? My inclination is yes.

I also think that is the most appropriate. We could wrap the value in an array, and then when a selection changes we would need to set an array. But that could lead to silent bugs for us overwriting data this way.
In short, throw an error if:

• multiple is true
• is not null/undefined
• is not an array

Not really sure how to tackle the add/remove actions here...

[cowboyd]

Isn't this "Ember 2.0" compliant? I don't know what work will be needed to become more compliant to "Data down, actions up" than this.

Honestly, the more I think about it, the less I think I know what "one-way" means in ember 2.0, and I think any speculation is not helping us move forward. I have a sneaking suspicion that the answer to my question is that value={{anArray}} will still allow components to mutate the contents of the array, but it's possible also that in 2.0, calling setObjects() will not, in fact do anything to the original model. In any case, let's move on with what we know to be true now.

I think this PR included that other PR. I started from that point. Is that enough?

That's odd. Those changes shouldn't then be showing in the diff, maybe it won't be a problem.

Let's go ahead and add the exception, and I'll add the tests for it as well as the multi-select

Not really sure how to tackle the add/remove actions here...

On 'change', we'll have to perform a diff between the new array and the old one and raise either the add or remove actions, but let's do that in the next PR after we ship this, which hopefully isn't too far away.

[cowboyd]

I've updated the tests, added a section to the readme, and release v1.1.0 Thanks @miguelcobain!

[miguelcobain]

Great! ⛵

In the example there's an error on the closing {/x-select}} (missing curly brace).

Regarding your "heads up" in the readme, I remind you that we no longer change the array, we use setObjects: https://github.com/thefrontside/emberx-select/blob/master/addon%2Fcomponents%2Fx-select.js#L120
I'm not really into how it is implemented, though.

I'm not having any trouble with ember-data's relationships here. :)
(except for the async ones, a known issue in Ember.Select for a long time).

Also

using computed using arrays and/or ember-data

Perhaps there's a duplicade "using" there. :)

[cowboyd]

Thanks for catching those typos.

We use setObjects, which changes the contents of the value array.

The reason I still included the warning is this: The biggest problem with using ED hasManys is that you cannot rollback, so any "cancellation" of changes must reload the record from the server. It's just been our experience that you're always better off just using a simple array, and then copying over your changes when you're ready to commit.

[miguelcobain]

Alright. 👍