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

checkboxUncheckedValue and arrays #25

Closed
KATT opened this issue Oct 6, 2014 · 14 comments
Closed

checkboxUncheckedValue and arrays #25

KATT opened this issue Oct 6, 2014 · 14 comments

Comments

@KATT
Copy link
Contributor

KATT commented Oct 6, 2014

I ran into an issue with combining checkboxUncheckedValue and array-like checkboxes

<label class="checkbox-inline">
  <input type="checkbox" name="flags[]" value="input1"> Input 1
</label>
<label class="checkbox-inline">
  <input type="checkbox" name="flags[]" value="input2"> Input 2
</label>

It's currently getting treated as hash, i.e.

{ 
  "flags": {
    "input1": false,
    "input2": true
  }
}

It would make sense if they were defined as name="flags[input1]" but in this case it should rather be parsed as an array.

Expected result

No checked boxes:

{ 
  "flags": []
}

Checked boxes:

{ 
  "flags": ["input2"]
}
@MarioLinden
Copy link

I am assuming that you are checking the "input2" checkbox and then serializing with something like:

$('input').serializeJSON(checkboxUncheckedValue: "false", parseBooleans: true);

I agree the expected result should be an array, not a hash.

But with the checkboxUncheckedValue option, the "false" values should be included.

Expected result with only "input2" checked:

{ 
  "flags": [false, "input2"]
}

Don't you agree with this?

@KATT
Copy link
Contributor Author

KATT commented Oct 6, 2014

Hmm. In terms of checkboxUncheckedValue and Arrays I think it only should make sure that the array is always posted, even if no checkbox is checked, as I described above.

Let's say you do a HTTP PATCH request to update which flags to use & no boxes was checked; jQuery's built-in serialize() (& serializeJSON with checkboxUncheckedValue: false) would not even include flags: [] in the request. I think checkboxUncheckedValue's only responsibility in the above case is to make sure that flags[] is always sent, even if it's empty.

@marioizquierdo
Copy link
Owner

I have to think about it.
It seems to me that not including the unchecked values in the array could be confusing. Also, including them also ensures that the array is always submitted, and it should not be hard to filter out the unchecked values server side. The problem might be with the option name checkboxUncheckedValue that suggest to include a value for non checked checkboxes. Think of it as adding a data-unchecked-value on each checkbox... they should be included right?
After all, the original specification would not even send an array.

@KATT
Copy link
Contributor Author

KATT commented Oct 6, 2014

If you do an update of an array of strings you pass in the whole lot, as there's no way of distinguishing what is removed otherwise. You can solve this with an object but it's not really RESTful.

Also, since you also need to know if all the entries has been removed you also need to pass in an empty array. This should be the role of checkboxUncheckedValue as otherwise you get the same issues with normal checkboxes, where the standard spec just tells you not to send it.

KATT added a commit to KATT/jquery.serializeJSON that referenced this issue Oct 6, 2014
KATT added a commit to KATT/jquery.serializeJSON that referenced this issue Oct 6, 2014
@KATT
Copy link
Contributor Author

KATT commented Oct 6, 2014

.. but obviously you don't have to agree to me :)

@marioizquierdo
Copy link
Owner

You know KATT, there's a blur line between more features and simplicity. If a problem becomes to complex it is probably better to not support it and let users roll their own solution.

There is a clear case here where the array of checkboxes is serialized as an object. That needs to be fixed. But if I'm not sure about how to easily and clearly solve the problem, I prefer to recommend some workaround meanwhile and put a solution later when I'm convinced.

There is value in what you said, and I will probably go with it. It just still doesn't click the relation between the option and the behavior.

Time will say ...

KATT added a commit to KATT/jquery.serializeJSON that referenced this issue Oct 6, 2014
KATT added a commit to KATT/jquery.serializeJSON that referenced this issue Oct 6, 2014
@KATT
Copy link
Contributor Author

KATT commented Oct 6, 2014

Not a very elegant solution.. but at least I added some tests too.

Please code review dc74490 and get back to me what you think.

@KATT
Copy link
Contributor Author

KATT commented Oct 8, 2014

@marioizquierdo, have you had time to think about it more?

@marioizquierdo
Copy link
Owner

I'm thinking ... the option checkboxUncheckedValue is probably not properly explained. It is not an option to enable (true) or disable (false), but the value (string) to use for all unchecked checkboxes. Could only disable with (undefined). In which case I still think that not including those values for unchecked checkboxes will be confusing.

But there's value on sending an empty array or including only selected values.
I would probably solve it without checkboxUncheckedValue, with something like this instead:

<input type="hidden" name="flags" value="[]"> <!-- ensure always send a list -->
<input type="checkbox" name="flags[]" value="input1">
<input type="checkbox" name="flags[]" value="input2">
var parseEmpryArray = function(val){ return val == "[]" ? [] : val; };
$('input').serializeJSON({parseWithFunction: parseEmpryArray });

Otherwise, using the defaults and checking for existence of flags in the backend is still fine. That is actually a more standard way to solve the problem.

I will get into the code this weekend. Thanks!

@KATT KATT closed this as completed Oct 8, 2014
@KATT
Copy link
Contributor Author

KATT commented Oct 9, 2014

It was a good idea, only it don't work to do that (which was obvious when I think about it).

flags becomes "[]", not [].

@KATT KATT reopened this Oct 9, 2014
@marioizquierdo
Copy link
Owner

Yes it works. But you need no forget about the parseWithFunction option, to convert the "[]" (string) to a real [] (empty list).

I know it's still not an easy way to send an empty array.. I wish the name syntax would allow for something simpler.

@marioizquierdo
Copy link
Owner

The code at dc74490 is good, specially because is baked by tests (KATT@a0be870).
It would not work with more complex keys (i.e. "foo[][bar]"), but at least would work for your case.

Unfortunately I'm not merging it because I decided not to implement this functionality after all.
I agree there is a problem with the way checkboxes work by default in HTML forms but serializeJSON should not try to be over-smart and create empty lists in some cases depending in some combination of options. I think is better that people deal with the problem of the empty list server side, even if it breaks the semantics of a PATCH request. After all, that's how a normal form works and is a well known problem.

I prefer to keep around a well-known problem than introducing new hidden complications.

@marioizquierdo
Copy link
Owner

Interesting I was trying to reproduce the bug and I couldn't.

It is returning a list for me. With the expected behavior that I mentioned.

describe('checkboxUncheckedValue', function() {
    it('works on a list of checkboxes', function() {
        $form = $('<form>' +
            '<label class="checkbox-inline">' +
            '  <input type="checkbox" name="flags[]" value="input1"> Input 1' +
            '</label>' +
            '<label class="checkbox-inline">' +
            '  <input type="checkbox" name="flags[]" value="input2"> Input 2' +
            '</label>' +
            '</form>');
        obj = $form.serializeJSON({checkboxUncheckedValue: 'false'});
        expect(obj).toEqual({
            'flags': ['false', 'false']
        });

        $form.find('input[value="input1"]').prop('checked', true);
        obj = $form.serializeJSON({checkboxUncheckedValue: 'false'});
        expect(obj).toEqual({
            'flags': ['input1', 'false']
        });
    });
});

Are you sure you are not running an old version of Zepto or jQuery?

@marioizquierdo
Copy link
Owner

Note that from version 2.4.0 you could simplify the checkbox problem with the use of the :array type (instead of writing a custom parser function) like this:

<input type="hidden" name="flags:array" value="[]"> <!-- send empty list if all checkboxes are unchecked -->
<input type="checkbox" name="flags[]" value="input1">
<input type="checkbox" name="flags[]" value="input2">
$('input').serializeJSON(); // <-- no options needed

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

No branches or pull requests

3 participants