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

91% of formatted dates differ from native implementation #124

Open
vectart opened this Issue Jul 14, 2015 · 18 comments

Comments

Projects
None yet
5 participants
@vectart

vectart commented Jul 14, 2015

Since I had faced #117 formatting issue, I created a simple check suite of native and polyfill implementation and it revealed that 5313 from 5831 formatted dates are different from native ones (https://jsbin.com/wekebe/edit?js,output, check Chrome dev console)

intl_results

Could be the Intl.js improved to fit at least 50% of variations?

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy Jul 14, 2015

Collaborator

@vectart this is awesome. I will look into the details when I get a chance.

Collaborator

caridy commented Jul 14, 2015

@vectart this is awesome. I will look into the details when I get a chance.

@caridy caridy self-assigned this Jul 14, 2015

@caridy caridy added the enhancement label Jul 14, 2015

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy Jul 14, 2015

Collaborator

Note to self: exact same results are exhibited in firefox.

Collaborator

caridy commented Jul 14, 2015

Note to self: exact same results are exhibited in firefox.

@andyearnshaw

This comment has been minimized.

Show comment
Hide comment
@andyearnshaw

andyearnshaw Jul 14, 2015

Owner

Whilst fixing this, it might be worth including this code as a regression test suite with an automatic fail if we don't achieve above a certain threshold. I've been wanting to write some regression tests for a while now, it didn't occur to me to check all permutations against the native implementation that's shipped with node v0.12.

V8's Intl still fails a few of DateTimeFormat tests, but I think those are more or less superficial.

Owner

andyearnshaw commented Jul 14, 2015

Whilst fixing this, it might be worth including this code as a regression test suite with an automatic fail if we don't achieve above a certain threshold. I've been wanting to write some regression tests for a while now, it didn't occur to me to check all permutations against the native implementation that's shipped with node v0.12.

V8's Intl still fails a few of DateTimeFormat tests, but I think those are more or less superficial.

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy Jul 14, 2015

Collaborator

I do have a test suite locally that I was planning to commit. Will try to get that in soon. @jasonmit was also going to help with that.

Collaborator

caridy commented Jul 14, 2015

I do have a test suite locally that I was planning to commit. Will try to get that in soon. @jasonmit was also going to help with that.

@andyearnshaw

This comment has been minimized.

Show comment
Hide comment
@andyearnshaw

andyearnshaw Jul 15, 2015

Owner

Awesome, I look forward to seeing it in action. :-)

Owner

andyearnshaw commented Jul 15, 2015

Awesome, I look forward to seeing it in action. :-)

@vectart vectart changed the title from 91% of formatted dates are different from native implementation to 91% of formatted dates differ from native implementation Jul 15, 2015

@vectart

This comment has been minimized.

Show comment
Hide comment
@vectart

vectart Jul 15, 2015

Yes, it would be great to see in test results something more meaningful than

9.2.1_1
9.2.1_3
9.2.1_4
9.2.1_8_c_ii
9.2.1_8_c_vi
9.2.2

vectart commented Jul 15, 2015

Yes, it would be great to see in test results something more meaningful than

9.2.1_1
9.2.1_3
9.2.1_4
9.2.1_8_c_ii
9.2.1_8_c_vi
9.2.2
@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy Jul 15, 2015

Collaborator

@vectart those are generated tests for every step of the algo based on the specifications, we will just add other tests to verify cldr data, and compare with native.

Collaborator

caridy commented Jul 15, 2015

@vectart those are generated tests for every step of the algo based on the specifications, we will just add other tests to verify cldr data, and compare with native.

@vectart

This comment has been minimized.

Show comment
Hide comment
@vectart

vectart Jul 15, 2015

Seems like these generated tests are absolutely useless

vectart commented Jul 15, 2015

Seems like these generated tests are absolutely useless

@andyearnshaw

This comment has been minimized.

Show comment
Hide comment
@andyearnshaw

andyearnshaw Jul 15, 2015

Owner

@vectart: those tests are incredibly useful. Although they don't assume any specific locales are supported (or that we're even using CLDR), they ensure that the bare minimum of required date/time formats are supported, the correct options are resolved for formatters, tags are properly canonicalised and matched and much more.

That being said, Intl.js has sorely needed some tests to prevent locale data-based regressions for a long time.

Owner

andyearnshaw commented Jul 15, 2015

@vectart: those tests are incredibly useful. Although they don't assume any specific locales are supported (or that we're even using CLDR), they ensure that the bare minimum of required date/time formats are supported, the correct options are resolved for formatters, tags are properly canonicalised and matched and much more.

That being said, Intl.js has sorely needed some tests to prevent locale data-based regressions for a long time.

@vectart

This comment has been minimized.

Show comment
Hide comment
@vectart

vectart Jul 15, 2015

@andyearnshaw How was the bare minimum of required date/time formats defined if such basic formats doesn't work?
#117 { month: 'long' }
#126 { month: 'long' }
#125 { weekday: 'long' }

I really want to use Intl.js but seems it's better to fallback to Moment.js, Globalize or other more stable date-formatting library.

vectart commented Jul 15, 2015

@andyearnshaw How was the bare minimum of required date/time formats defined if such basic formats doesn't work?
#117 { month: 'long' }
#126 { month: 'long' }
#125 { weekday: 'long' }

I really want to use Intl.js but seems it's better to fallback to Moment.js, Globalize or other more stable date-formatting library.

@andyearnshaw

This comment has been minimized.

Show comment
Hide comment
@andyearnshaw

andyearnshaw Jul 15, 2015

Owner

From the spec, section 12.2.3:

The following subsets must be available for each locale:

  • weekday, year, month, day, hour, minute, second
  • weekday, year, month, day
  • year, month, day
  • year, month
  • month, day
  • hour, minute, second
  • hour, minute

Initially, we only supported one pattern for each of those subsets, the bare minimum to meet the spec's requirements, pass the tests and cover the majority of use cases. @caridy is working on improving the date formats—have a little patience (or feel free to contribute 😉).

Owner

andyearnshaw commented Jul 15, 2015

From the spec, section 12.2.3:

The following subsets must be available for each locale:

  • weekday, year, month, day, hour, minute, second
  • weekday, year, month, day
  • year, month, day
  • year, month
  • month, day
  • hour, minute, second
  • hour, minute

Initially, we only supported one pattern for each of those subsets, the bare minimum to meet the spec's requirements, pass the tests and cover the majority of use cases. @caridy is working on improving the date formats—have a little patience (or feel free to contribute 😉).

@oliviertassinari

This comment has been minimized.

Show comment
Hide comment
@oliviertassinari

oliviertassinari commented Aug 21, 2015

+1

@chilipote

This comment has been minimized.

Show comment
Hide comment
@chilipote

chilipote Oct 26, 2015

Some news about that ?

chilipote commented Oct 26, 2015

Some news about that ?

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy Feb 11, 2016

Collaborator

with the v1.1.0 release, we are down to 60%, big improvements. Now I'm seeing 3505 from 5831 failed in FF and Chrome.

Collaborator

caridy commented Feb 11, 2016

with the v1.1.0 release, we are down to 60%, big improvements. Now I'm seeing 3505 from 5831 failed in FF and Chrome.

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy May 12, 2016

Collaborator

with the v1.2.0 release, we are down to 0%, impressive. Now I'm seeing 0 from 5831 failed in FF and Chrome.

Collaborator

caridy commented May 12, 2016

with the v1.2.0 release, we are down to 0%, impressive. Now I'm seeing 0 from 5831 failed in FF and Chrome.

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy May 12, 2016

Collaborator

fixed by #171

update: this is not fixed yet.

Collaborator

caridy commented May 12, 2016

fixed by #171

update: this is not fixed yet.

@vectart

This comment has been minimized.

Show comment
Hide comment
@vectart

vectart May 12, 2016

Absolutely impressive! Thanks, everyone!

vectart commented May 12, 2016

Absolutely impressive! Thanks, everyone!

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy May 13, 2016

Collaborator

@vectart something happen when attempting to test your test, I ended up copying that as part of the automation process, and I still see a bunch of differences, some of them, the polyfill is doing the right thing, while browsers take shortcut, but in general, we will have to continue this effort to close the gap. that 0% was very smelly :(, but let's keep targeting something like 20%.

Collaborator

caridy commented May 13, 2016

@vectart something happen when attempting to test your test, I ended up copying that as part of the automation process, and I still see a bunch of differences, some of them, the polyfill is doing the right thing, while browsers take shortcut, but in general, we will have to continue this effort to close the gap. that 0% was very smelly :(, but let's keep targeting something like 20%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment