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

Custom types #40

Merged
merged 7 commits into from
Apr 24, 2015
Merged

Conversation

tygriffin
Copy link
Contributor

It would be really great to be able to define custom types like so:

$.serializeJSON({
    customTypes: {
        myType: function(value) { 
            // some custom serialization
        }
    }
});

The use cases I've encountered would be nullable types or date types where the time is stripped out.

@marioizquierdo
Copy link
Owner

I think this feature is great, and it doesn't seem to add much more complexity to the overall design. I think it can be explained in the README file easily, and anyone reading the code would get it.

The only thing... when I see the new implementation for parseValue, it almost seems like all types should be implemented as custom types. Instead of having a customTypes option, it could be better to just have a $.serializeJSON.types object, with something like this:

$.serializeJSON.types = {
  string:  function(str) { return String(str) },
  number:  function(str) { return Number(str) },
  boolean: function(str) { return (["false", "null", "undefined", "", "0"].indexOf(str) === -1) },
  null:    function(str) { return ["false", "null", "undefined", "", "0"].indexOf(str) !== -1 ? null : str },
  array:   function(str) { return JSON.parse(str) },
  object:  function(str) { return JSON.parse(str) },
  auto:    function(str) { $.serializeJSON.parseValue(str, null, {parseNumbers: true, parseBooleans: true, parseNulls: true}) } // try again with something like "parseAll"
}

parseValue: function(str, type, opts) {
  var f = $.serializeJSON;
  // Parse with a type if available
  if (type && f.types && f.types[type]) return f.types[type](str); // use specific type

  // Otherwise, check if there is any auto-parse option enabled and use it.
  if (opts.parseNumbers  && f.isNumeric(str)) return Number(str); // auto: number
  if (opts.parseBooleans && (str === "true" || str === "false")) return (["false", "null", "undefined", "", "0"].indexOf(str) === -1); // auto: boolean
  if (opts.parseNulls    && str == "null") return ["false", "null", "undefined", "", "0"].indexOf(str) !== -1 ? null : str; // auto: null
  return str; // otherwise, keep same string
},

extractTypeFromInputName: function(name) {
  // ...
  validTypes = Object.keys(f.types);
  // ...
}

Now the code is more self-explanatory and simple, where extending types becomes a non-issue, with the only drawback that it would be harder to scope different sets of types for different calls to serializeJSON, but I think that could be a rare case (and is still possible by replacing the value for $.serializeJSON.types in a per-call basis).

What do you think?

@tygriffin
Copy link
Contributor Author

Makes perfect sense to me. I'll have a go at updating the code, tests and readme in the next day or two.

@tygriffin
Copy link
Contributor Author

Ok, updated ⚡

@@ -15,6 +15,7 @@
var serializedObject, formAsArray, keys, type, value, _ref, f, opts;
f = $.serializeJSON;
opts = f.optsWithDefaults(options); // calculate values for options {parseNumbers, parseBoolens, parseNulls}
$.extend(f.types, opts.types); // extend default types with custom types
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, instead of overriding the global types, it should just use the global types as defaults.

opts.types = $.extend({}, opts.types, f.types); // set default types

This way, one could either modify the global types $.serializeJSON.types['mynewglobaltype'] = function(str){...} or define/override types just for the $(selector).serializeJSON({types: {mynewlocaltype: function(str){...}}}) call, without affecting the global namespace (and have unexpected side effects).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, of course! Done.

@marioizquierdo
Copy link
Owner

Wonderful!
I try to merge it and create a new release this weekend :)

marioizquierdo added a commit that referenced this pull request Apr 24, 2015
@marioizquierdo marioizquierdo merged commit 873754b into marioizquierdo:master Apr 24, 2015
@marioizquierdo
Copy link
Owner

@tygriffin Ok, this has been merged and released as the version 2.6.0.

I added a few more changes in this commit 53842f4

The final interface allows to define both customTypes and defaultTypes, both as options in the serializeJSON call and as $.serializeJSON.defaultOptions.

So, the simple case will be exactly like your first suggestion:

$.serializeJSON({
    customTypes: {
        myType: function(str) { 
            // some custom serialization
        }
    }
});

But now it's easy to define custom types or just re-define all types (with defaultTypes).

Thanks for the pull request!

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

Successfully merging this pull request may close these issues.

2 participants