Modify attrs.js to accept Date strings in custom element attribute values

[bsatrom]

I'm working on a custom element that's essentially a component implementation of a Calendar widget. So far, I've defined three attributes on that element, value, min and max, all of which take a Date string. So, in practice, the component could be initialized like so:

<k-calendar value="May 23, 2013"></k-calendar>

or

<k-calendar value="2013-05-23"></k-calendar>

etc.

The first case works just fine, and the full value is provided to my component upon registration. The latter value (or any numeric date string) doesn't, and only the year segment is provided to my component. After digging a bit, I discovered the cause, the deserializeValue method in attrs.js. Because this method is only looking for bools and numbers, and numeric date string is parsed as a float and passed back to my component.

I've made a change to attrs.js in my fork of the toolkit repo, but I figured I'd open an issue for discussion before sending a PR. The change I made was to add the following before the parseFloat call:

// Check for date values (YYYY[./-]MM[./-]DD format)
if (inValue.match(/^\d{4}[./-]\d{2}[./-]\d{2}$/g)) {
  return inValue;    
}

Recognizing that this might not be the best regex for the job, I'm happy to discuss this further and modify as-needed before sending in a PR, assuming you all agree that this is a valid issue.

Thanks!

[sjmiles]

We bumped into this right about the same time you did, although in a couple of different cases. In particular, strings like "48px" were also deserialized as numbers.

There is already a fix checked into [master], although there is some continuing debate about the best way to handle deserialization.

I hadn't considered supporting Date as an intrinsic type. I'm sure you've noticed that we have a gambit for inferring data types, I suppose we could use that to return Date objects for values that have Date-valued defaults.

[bsatrom]

I tested your fix, and it solves my issue as well, without adding any date-specific intelligence, so that's nice.

as for supporting dates, I think it's probably okay to not return Date objects. Seems like it would complicate things to have to handle all string formats that parse into a valid Date object.

Along those lines, what's the value in performing any kind of deserialization of types on attributes? Is this something detailed in the Custom Elements spec (I couldn't find any mention of custom element attributes) or mailing list discussions? Just curious about the use-case for delivering these types directly to component devs, as opposed to letting them handle type conversion based on the needs of the component.

I'll close this issue since I think it makes sense for date strings to be passed into a component, as is.

[sjmiles]

Notice that (mostly) the idea is that we attempt to preserve the type the component has already established.

So if you declare 'foo: false', the deserializer will make sure foo remains a Boolean. This is important because true boolean attributes have a value of '' (empty string) which is falsey in JavaScript (this can be very confusing).

The gambit would be the same for Date. In other words, we wouldn't try to figure out if it's a date from looking at the string, instead, if the value in the component is a Date to start out with, we would make sure it stays that way.

I realize this is a bit confusing because we use a slightly different strategy for numbers, which is to say we do actually try to determine the type from the string itself. This is supposed to be a convenience, but I'm not sure it's the right strategy.

Thanks for the thoughtful feedback.

[bsatrom]

ah, well that makes a lot more sense, thanks!

I saw that you just created an issue for adding dates to the gambit. I'm happy to take a stab at that one as a first PR, if it's not already taken.

[sjmiles]

Stab away!

Please just make sure to complete a CLA if you haven't already, as described here https://github.com/toolkitchen/toolkit/blob/stable/CONTRIBUTING.md.

[bsatrom]

great. CLA is already signed, so I should be good there.

thanks.