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

Some values are lost if an array contains both integers and strings as keys #95

Open
hingst opened this issue Oct 19, 2016 · 4 comments
Labels

Comments

@hingst
Copy link

hingst commented Oct 19, 2016

Say you have three text inputs:

<input type="text" name="myname[0]">
<input type="text" name="myname[1]">
<input type="text" name="myname[stringkey]">

When serializing this form, the first two fields are lost in the resulting object. I think the reason is in lines 62 through 70. In the first two iterations myname is detected as "fixed" (line 64), while in the third iteration it's detected as "named" (line 69). This leads to the first two values being overwritten by the third.

@macek
Copy link
Owner

macek commented Oct 20, 2016

This is unsupported because the original intention was to support actual arrays. After building myname[0] and myname[1] there will be an array in memory. When the serializer gets to myname[stringkey] it would have to convert that array to an object which would result in wasted work.

There are many ways to solve this problem. A few I can think of are:

possible option: data-* attributes

<input type="text" name="myname[0]" data-serialize="object">
<input type="text" name="myname[1]">
<input type="text" name="myname[stringkey]">

possible option: schema – leave the form the same but pass schema to serializer

$('#form').serializeJSON({
  somearray: FormSerializer.array,
  myname: FormSerializer.object
});

possible option: builder object


let builder = {
  array: {empty: [], builder: function(acc, key, value) { return [...acc, value]; }},
  object: {empty: {}, builder: function(acc, key, value) { return Object.assign(acc, {[key]: value}); }}
};

$('#form').serializeJSON({
  somearray: builder.array,
  myname: builder.object
});

The builder option is obviously the most powerful because any custom function could be used. This would allow the user to do things like

$('#form').serializeJSON({
  // convert array of strings to a flat string
  arrayOfStrings: {empty: '', builder: (acc,key,value) => acc + value]}
  // convert strings to numbers
  arrayOfNumbers: {empty: [], builder: (acc,key,value) => Object.assign(acc, {[key]: parseInt(value, 10)})}
})

Anyway, I haven't made time to work on this project in a while, so if anyone wants to contribute ideas or implementations here, that'd be helpful.

@macek macek added the feature label Oct 20, 2016
@evgenyvas
Copy link

I had this problem too and I decided to add a new option.
Maybe it's an ugly solution, but it's works.
You can use numkeys option from my fork:

$("form").serializeObject({'numkeys':true});

evgenyvas@df7871c

@neofly
Copy link

neofly commented Dec 1, 2016

Hello,

I think the correct solution is to convert Array to Object. It's not wasted work, it's the consequence of your lazy algorithm.

When the program see a bracket with numeric value, it's need to determine if it's a array i.e with consecutive and numeric key. Without the knowledge of all keys, it can't determine it. Naive approach is to loop over all keys. Lazy approach is to take the less costly choice until proven otherwise. In this case, conversion is not wasted work, it's the work you have chosen not to do immediately with lazy approach. Correct lazy algorithm need to convert properly, so no data is lost.

So even with your hint, you need to correct your algorithm. In my opinion, you hint is premature optimization. JavaScript is fast enough to transform array to object in most cases.

@macek
Copy link
Owner

macek commented Dec 6, 2016

@neofly

Don't overlook the fact that this kind of mixing behaviour is explicitly defined in the tests and has been discouraged for a long time

https://github.com/macek/jquery-serialize-object/blob/master/test/unit/add-pair-test.js#L49-L55

  it("should punish user for mixing pushed array and fixed array fields", function() {
    f.addPair({name: "a[]",  value: "b"});
    f.addPair({name: "a[2]", value: "c"});
    f.addPair({name: "a[]",  value: "d"});
    f.addPair({name: "a[5]", value: "e"});
    assert.deepEqual(f.serialize(), {a: ["b", "d", "c", , , "e"]});
  });

This isn't the exact case we're talking about, but it's under the same umbrella. Mixing types is frowned upon and makes it harder for the serializer to figure out what the correct output should be. I'm have a greater appreciation for type inference than I did 3 years ago, but I still mostly prefer explicit over implicit behavior.

Also consider the fact that the plugin is essentially 3 years old and didn't understand it's use case perfectly at the time of its conception. Since then there have been countless discussions about what improvements can be made to the underlying serializer, but few people have been willing to contribute the hard work to make effective changes - not the tiny incremental patches that only make the serializer more complex and increase its fragility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants