Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Add number fields and dist #3

Merged
merged 2 commits into from
Nov 25, 2018
Merged

Conversation

snewcomer
Copy link

@snewcomer snewcomer self-assigned this Nov 20, 2018
@@ -34,7 +34,8 @@ module.exports = function (grunt) {

extract_cldr_data: {
options: {
pluralRules: true
pluralRules: true,
numberFields: true
Copy link
Author

Choose a reason for hiding this comment

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

hmmm. Seems this will increase the bundle size quite a bit...

Copy link
Member

@jasonmit jasonmit Nov 25, 2018

Choose a reason for hiding this comment

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

Thanks for calling that out.

For our ember bundles, we pay as you go since we import only the locale data that's targeted. Not terribly concerned.

Copy link
Author

@snewcomer snewcomer Nov 26, 2018

Choose a reason for hiding this comment

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

@jasonmit Yeah that is a good point! The only thing I noticed is we are duplicating pluralRuleFunction for a particular lang. One in the vendor dist and the other in the app bundle.

I'm not 100% sure the locale data for this lib is needed in the vendor.js file, but perhaps you know much better than I do. My understanding was that at runtime, we handoff the relevant data generated from broccoli-cldr-data to IntlMessageFormat. Don't see us requireing this data from intl-messageformat anywhere. Lmk your thoughts!

Copy link
Member

@jasonmit jasonmit Nov 26, 2018

Choose a reason for hiding this comment

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

The only thing I noticed is we are duplicating pluralRuleFunction for a particular lang. One in the vendor dist and the other in the app bundle.

Sounds like a bug, this is happening on master & easy to reproduce? If so, will investigate soon.

I could see where this could be the case for en but would not expect it for any other locales (lib defaults https://github.com/yahoo/intl-messageformat/blob/master/dist/intl-messageformat.js#L1921 https://github.com/yahoo/intl-relativeformat/blob/master/dist/intl-relativeformat.js#L1879)

Copy link
Author

Choose a reason for hiding this comment

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

Oh you are right! Other languages does not add anything to the vendor output. 👍

@jasonmit jasonmit merged commit 9e6bf35 into ember-intl:master Nov 25, 2018
@jasonmit
Copy link
Member

@snewcomer as always, version accordingly :)

@snewcomer snewcomer deleted the sn/number-fields branch November 26, 2018 18:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants