Calendar: Using globalize-compile

[fnagel]

This PR introduces compiled formatters & parsers by using globalize-compile. It's using the lastet 1.1 rc6 of globalize.

We probably want to have the Grunt compile-globalize task to be more flexible -- not sure if we need to process other widgets dependencies with this.

Big thank you to @rxaviers for his help how to do this and quick bugfixes!

[fnagel]

@jzaefferer @scottgonzalez @rxaviers

Ready for a first review!

[scottgonzalez]

@rxaviers Do you have an ETA on 1.1.0?

[rxaviers]

Nope. Technically the rc.6 is pretty close to 1.1.0. But, writing a blog post about it (for the release) is the biggest uncertainty on my end.

[jzaefferer]

Can I help with that? Is that the blog post I've reviewed before?

[rxaviers]

The post you've reviewed is certainly a big portion of it, but 1.1.0 should include more (e.g., unit formatting). You are welcome to help. :)

[scottgonzalez]

Overall, this looks good to me. The actual changes are fairly small :-)

[fnagel]

Fixed the jshint tests.

[scottgonzalez]

I'd say this is good to land.

[jzaefferer]

While this package is pretty convenient, it also blows up an installed node_modules folder by 250 to 300 MB. @rxaviers is there any way to get an English-only cldr-data, since that's all we care about here (I think)?

[fnagel]

I assume we would need at least the locales defined here: https://github.com/jquery/jquery-ui/pull/1651/files#diff-35b4a816e0441e6a375cd925af50752cR408

[jzaefferer]

Nevermind, we're actually compiling several locales.

[jzaefferer]

Another idea: Can we install cldr-data as part of the compile-globalize grunt task? That'll make the task run a little longer, but avoid a lot of overhead for every other CI run and local install. Since we're committing the result to the repo, it seems pointless to install the dependency all the time.

[rxaviers]

Answering the first questions, although I see your line of thought is going somewhere else already...

@rxaviers is there any way to get an English-only cldr-data, since that's all we care about here (I think)?

No, cldr-data allows you to choose one of the coverage options given by CLDR (for instance, modern or full only).

[jzaefferer]

Also reviewed the whole thing. Looks like the issue in tests/unit/date/helper.js is the only one that needs to be addressed before this can land, everything else can happen elsewhere.

[scottgonzalez]

Looks good.

[fnagel]

Just checking why the unit tests fail...

[fnagel]

Mhh, this seems like a bug. Unit test does not fail when system date is November 30th.
Sadly these tests are not really documented. @rxaviers Any idea regarding the bug or the tests?

https://travis-ci.org/jquery/jquery-ui/builds/94433780#L257-L261
https://github.com/fnagel/jquery-ui/blob/datepicker-globalize/tests/unit/date/core.js#L160-L171

[fnagel]

@scottgonzalez Merging this anyway, ok? Issue seems unrelated to the actual changes in this PR.

[jzaefferer]

Would help to know which of the 5 assertions in that test is failing. The stack should tell, but only points at qunit.js.

Is the test failing without the changes from this branch? Then merging anyway seems fine.

[rxaviers]

Yeap, knowing which assertion (of the 5) is failing would help. Also, is a Globalize call providing a wrong result? If so, knowing which would help me to provide a quicker response.

[fnagel]

The last test in this set is failing. Just tested this, fails on current datepicker branch too.

[fnagel]

This test to be specific: https://github.com/fnagel/jquery-ui/blob/datepicker-globalize/tests/unit/date/core.js#L170

[fnagel]

Also, is a Globalize call providing a wrong result?

There are no console errors or similar issues. Anything else I could check?

[jzaefferer]

Let's merge then.

You can rewrite the assertion to get more useful output:

equal( firstMonth.month(), lastMonth.month() - 1 ); 

Also noticed that this against $.date(), so it'll change with the system time. Looks like other tests avoid that by calling setFullDate, for example date.setFullDate( 2012, 9, 16 );