Skip to content

refactor(common): refactor i18n code to ease tree shaking#18857

Closed
vicb wants to merge 2 commits intoangular:masterfrom
vicb:0823-intl
Closed

refactor(common): refactor i18n code to ease tree shaking#18857
vicb wants to merge 2 commits intoangular:masterfrom
vicb:0823-intl

Conversation

@vicb
Copy link
Copy Markdown
Contributor

@vicb vicb commented Aug 24, 2017

The former structure was causing a circular dependency error in g3.
This PR re-organize the code and "fixes" minor issues to remove this CD.

Please review the first commit only.
(You can check the 2nd but it's only updating generated files)

@vicb vicb added area: i18n Issues related to localization and internationalization action: review The PR is still awaiting reviews from at least one requested reviewer feature Label used to distinguish feature request from other issues labels Aug 24, 2017
* @experimental i18n support is experimental.
*/
export function registerLocaleData(data: any, extraData?: any) {
const localeId = toCamelCase(data[LocaleDataIndex.LocaleId]);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ocombe Do we really need those toCamelCase calls ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No we can probably remove this

@mary-poppins
Copy link
Copy Markdown

You can preview e9e8bfa at https://pr18857-e9e8bfa.ngbuilds.io/.

* @experimental i18n support is experimental.
*/
export function registerLocaleData(data: any, extraData?: any) {
const localeId = toCamelCase(data[LocaleDataIndex.LocaleId]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No we can probably remove this


throw new Error(
`Missing locale data for the locale "${locale}". Use "registerLocaleData" to load new data. See the "I18n guide" on angular.io to know more.`);
throw new Error(`No data registered for the locale "${locale}".`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change in the error message was asked by @StephenFluin. Since this is probably something that any dev who doesn't use en-US will see, it's important that they don't feel lost with a non-informative error message

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@StephenFluin The longer the message the higher the payload. The right solution is to have error code as in AngularJS. Until then we should restrict from using longer than needed msgs IMO.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's actually a good idea, we should have error codes. I'll submit the idea

Two = 2,
Few = 3,
Many = 4,
Other = 5,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Zero = 0 is enough here, the other ones will be 1 to 5 since they follow 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

enough but not explicit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess it doesn't matter, typescript will add the numbers anyway

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It matters. It is a signal not to add a value in the middle.

@vicb vicb added the target: major This PR is targeted for the next major release label Aug 24, 2017
@vicb
Copy link
Copy Markdown
Contributor Author

vicb commented Aug 28, 2017

Note: the Travis failure is unrelated to this PR

@vicb
Copy link
Copy Markdown
Contributor Author

vicb commented Aug 28, 2017

will be merged via #18907

@vicb vicb closed this Aug 28, 2017
@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer area: i18n Issues related to localization and internationalization cla: yes feature Label used to distinguish feature request from other issues target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants