Skip to content
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

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

Closed
wants to merge 2 commits into from

Conversation

vicb
Copy link
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 action: review The PR is still awaiting reviews from at least one requested reviewer feature Issue that requests a new feature labels Aug 24, 2017
* @experimental i18n support is experimental.
*/
export function registerLocaleData(data: any, extraData?: any) {
const localeId = toCamelCase(data[LocaleDataIndex.LocaleId]);
Copy link
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
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

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
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

@@ -539,8 +515,7 @@ export function findLocaleData(locale: string): any {
return match;
}

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
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
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
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
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
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
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
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
Contributor Author

vicb commented Aug 28, 2017

Note: the Travis failure is unrelated to this PR

@vicb
Copy link
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

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 cla: yes feature Issue that requests a new feature target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants