Add custom date parsing module & Array deserialization support

[bsatrom]

There's a few things going on in this PR, so I'll break down the highlights. Let me know if I need to break these apart (if you want the date module, but not array support, etc.) of if I need to squash these into one commit.

1. Added support for Deserializing to Array from attributes, as per Consider deserializing to Array from attributes, if property is Array-valued #124. I didn't get a yea or nay on this one, so feel free to let me know if its a silly idea you all don't want to support, but it would come in handy for the components I'm building.
2. Refactored the deserializeValue method in attrs.js. Things were starting to look a bit heavy with four types in the switch, so I factored the switch into a handler object, and cleaned things up a bit. FWIW, the handler object does perform a bit better than switch when running the tests on my machine.
3. Added a module for custom date parsing (parseDates.js) as per Consider deserializing to Date from attributes, if property is Date-valued #119 and added deserialization of Date attributes for custom elements #122. This allows the deserializer to support dash and dot dates.

Once again, I know I crammed a few things in here, so let me know if I need to break things up to make life a bit easier on you all as you review and accept/reject.

Update: forgot to mention, but I also did some JSHint-motivated cleanup (whitespace, semicolons, etc.) in attrs.js.

[sjmiles]

Sorry for the delays we are all preparing for I/O.

So, rather than parsing Array specifically, if we want to go there, I suggest we support arbitrary objects via JSON.

For example, JSON.parse('[2, 3, 4]') == [2, 3 4]

IOW, if 'inferredType' is Object, would be nice if we could run the string through JSON.parse.

The problem there is that JSON strictly requires " (double-quote) so we might have to do some string manipulation (as users are likely to use ' (single-quote) in HTML).

Fwiw, 'typeHandlers' as written could be static, instead of regenerated inside the function.

However, having said that, I'm fairly sure switch is faster in general than hash-lookups, I hear you saying you tested otherwise, I'm not sure how to explain that.

Let me know your thoughts, sorry I can't be more complete, as I say, we are super busy this week.

[bsatrom]

No worries, I figured you all were pretty heads down this week. I like the idea of arbitrary object support via JSON. that would address my case, and many others, I presume. I'll revise that one accordingly.

As for switch/hash. I thought the same thing myself, but hash kept coming back faster from karma. Let me take those tests offline and into jsperf and see what's really going on.

In the meantime, I'll back out the refactor and array support stuff and resubmit this PR with just the Date.parse improvements, assuming that piece looks good.

[bsatrom]

Hey Scott,

I pulled out the date parsing module into a separate PR.

As for switch vs. lookups, it would seem that lookups are faster than switch statements, after all. See: http://jsperf.com/if-switch-lookup-table. On most browsers, it's 2x faster, and on newer ones, 3x or more. So I think it makes sense to use a lookup table. If you agree, I can pull it out of the function, as you suggested.

[arv]

If you move the lookup table outside the function I think it is fine. The way it is now, it gets created on every call which is a lot slower (and has a bad GC churn as well). http://jsperf.com/if-switch-lookup-table/11

On the other hand this is not hot code so readability trumps micro benchmarks. Premature optimization and all that...

[bsatrom]

Agreed. I was referring to perf as a static lookup. Faux pas on my part to include the lookup in the function, to begin with.

[sjmiles]

Ok, well that's good. I prefer the lookup pattern anyway.

Also suggest we add the fall-through code into the function map and default to it, something like:

if (!(inferredType in typeHandlers)) {
  inferredType = 'any';
}
typeHandlers[inferredType](...)

[arv]

FYI, in is slow (but like I said before; performance is not an issue here).

[sjmiles]

var handler = typeHandlers[inferredType] || typeHandlers.any;
handler(...)

[bsatrom]

If we add an 'object' handler to support Arrays and other types, we can put the fall-through code there. Or at least some of it. JSON.parse converts boolean strings, so that obviates the need for the "true|false" string check. The float parsing can go at the end of the object handler.

At least, that's how it's looking to me. I'll update this PR and you all can review and tell me if I'm missing something. All tests are passing in what I'm about to push.

[arv]

Hold... we should use boolean attributes for booleans. It is part of the
best practice document we have somewhere... let me dig it up.

On Mon, May 13, 2013 at 5:18 PM, Brandon Satrom [email protected]:

If we add an 'object' handler to support Arrays and other types, we can
put the fall-through code there. Or at least some of it. JSON.parse
converts boolean strings, so that obviates the need for the "true|false"
string check. The float parsing can go at the end of the object handler.

At least, that's how it's looking to me. I'll update this PR and you all
can review and tell me if I'm missing something. All tests are passing in
what I'm about to push.

—
Reply to this email directly or view it on GitHubhttps://github.com/toolkitchen/toolkit/pull/126#issuecomment-17841370
.

erik

[sjmiles]

Arv: this is where we support Boolean attributes.

[sjmiles]

In general, I like to use lightweight tests where we can and fall through to heavier tests.

So I prefer testing for 'true' and 'false' strings explicitly over the regex or using JSON.parse. Let's only fall though to JSON.parse when we have inferred an object type.

[arv]

https://docs.google.com/document/d/1lbWrU0qsGMijDwzYNttUw352lUhfU7DJW4yxA_A8hq4/edit?usp=sharing

Attributes for data in Use attributes to pass configuration in. Use boolean attributes for boolean values instead of something like selected="true".

[bsatrom]

sorry guys, jumped the gun. The main refactor is done in what I've just pushed, but it looks like obj/boolean support still requires some tweaking.

@arv I've seen that line in the doc before, and I'm not sure I follow. Does that imply that the attribute should be "selected" and not "selected=true"?

[arv]

@bsatrom It means that the value of the attribute has no effect. Just the presence of the attribute matters.

<x-foo a b="" c="true" d="false"></x-foo>

All of a, b, c and d are considered true.

[bsatrom]

@sjmiles Makes sense, though the challenge here is those cases where an attribute is supported, but no default is provided. In these cases, the defaultValue is null, so the the inferredType is object The tests in take-attributes.html on lines 127-139 illustrate this.

If we add a null-check for these cases, what do you think the appropriate next course of action should be? Check for true/false and attempt to parse float, as was done originally.

[sjmiles]

Check for true/false and attempt to parse float, as was done originally.

Yes. I think that's right.

[bsatrom]

@arv, thanks for clarifying. Makes sense.

@sjmiles ok, sounds good.

[bsatrom]

[FYI] okay, I'm back on the case with this one (after taking a break for I/O... great job btw guys!) and should have this pull cleaned up and finalized in the next day or so, along with Polymer/platform#13

[bsatrom]

Heads up guys, I'm about to push these changes under a new PR (mainly since the downstream branch I created is now named incorrectly), so we can close this one.

[sjmiles]

Thanks again for being so patient with all our nitpicking. :)

[bsatrom]

Hey, no problem! thanks for letting me contribute. I don't mind the nit-picking at all. Some great discussion.