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(ivy): i18n - factor out localization utils for re-use #32488

Closed

Conversation

@petebacondarwin
Copy link
Member

commented Sep 5, 2019

This PR moves some code around and sets us up for adding compile-time inlining and message id support.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mhevery
mhevery approved these changes Sep 6, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

FYI, VE and Ivy presubmits are successful.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-refactor-utils branch from df00c2f to 16da9f7 Sep 7, 2019
@petebacondarwin petebacondarwin requested a review from angular/dev-infra-framework as a code owner Sep 7, 2019
@gkalpak
gkalpak approved these changes Sep 7, 2019
Copy link
Member

left a comment

LGTM for dev-infra with one minor comment/question).

Not directly related to this PR:
Is it intentional that the localize package is not included in angular.io API docs?

packages/localize/utils/BUILD.bazel Outdated Show resolved Hide resolved
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2019

@gkalpak - thanks for the review:

Is it intentional that the localize package is not included in angular.io API docs?

For v9 we are aiming only at supporting i18n in Angular templates, rather than in user code (even if that is possible). Therefore, we are not "advertising" it in the angular.io docs quite yet. Also, since it is a global, I am not quite sure how it would fit into the current API documentation structure...

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-refactor-utils branch 4 times, most recently from d7321c8 to 3367cd9 Sep 8, 2019
Copy link
Member

left a comment

lgtm

btw the problem with codeowners is that we haven't setup codeowners for //packages/localize

@petebacondarwin can you please do so?

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

@IgorMinar - LOL - look what I did this morning: #32570

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-refactor-utils branch from 12e5207 to 3921a8a Sep 11, 2019
@IgorMinar

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

@angular/localize/src/utils is a deep import an won't be resolved correctly once we build stuff for differential loading as esm5 vs esm2015.

the rest was discussed in person

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

After face-to-face discussion we agreed to do the following:

  1. move the global side-effect import from the primary entry-point to a secondary entry-point @angular/localize/init.
    This has two benefits: first it allows the top level entry-point to contain tree-shakable shareable code; second it gives the side-effect import more of an "action" oriented name, which indicates that importing it does something tangible

  2. move all the source code into the top src folder, and import the localize related functions into the localize/init/index.ts entry-point.
    This allows the different parts of the package to share code without a proliferation of secondary entry-points (i.e. localize/utils).

  3. to avoid publicly exporting any utilities at this time - the only public API will be the global $localize and the two runtime helpers loadTranslations() and clearTranslations().
    This does not mean that we will not expose additional helpers for 3rd party tooling in the future, but avoid us preemptively exposing something that we might want to change in the near future.

After implementing this approach, I found one small issue:

Is that it was not possible to have the $localize code in the same Bazel package as the rest of the code. If we did this, then the @angular/localize/init entry-point code would contain all of the helper code, even though most of it was not used.

Equally it is not possible to have the $localize types (i.e. LocalizeFn and TranslateFn) only in the @angular/localize/init entry-point because these types are needed for the runtime code, which is inside the primary entry-point, and importing from @angular/localize/init will run the side-effect.

So the solution is to have a Bazel sub-package at //packages/localize/src/localize containing these types and the $localize function implementation. The primary //packages/localize entry-point can then import the types without any side-effect, the secondary //packages/localize/init can import the $localize function and attach it to the global scope as a side-effect, without bringing with it all the other utility functions.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-refactor-utils branch from 3921a8a to cd765d6 Sep 12, 2019
@petebacondarwin petebacondarwin requested a review from angular/fw-core as a code owner Sep 12, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-refactor-utils branch from cd765d6 to faa9fd7 Sep 12, 2019
@petebacondarwin petebacondarwin requested a review from angular/fw-public-api as a code owner Sep 12, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-refactor-utils branch 3 times, most recently from 74ade86 to 3aa4965 Sep 12, 2019
@petebacondarwin petebacondarwin requested a review from angular/fw-integration as a code owner Sep 12, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-refactor-utils branch from 3aa4965 to b3e5f26 Sep 12, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

FYI, VE and Ivy presubmits look good.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Note to Caretaker: this PR requires additional CL to be patched on g3 side due to the fact that @angular/localize is changed to @angular/localize/init and expanding rows benchmark depends on it (see changes in this PR). Thank you.

This is a refactoring that moves the source code around to provide a better
platform for adding the compile-time inlining.

1. Move the global side-effect import from the primary entry-point to a
   secondary entry-point @angular/localize/init.

   This has two benefits: first it allows the top level entry-point to
   contain tree-shakable shareable code; second it gives the side-effect
   import more of an "action" oriented name, which indicates that importing
   it does something tangible

2. Move all the source code into the top src folder, and import the localize
   related functions into the localize/init/index.ts entry-point.

   This allows the different parts of the package to share code without
   a proliferation of secondary entry-points (i.e. localize/utils).

3. Avoid publicly exporting any utilities at this time - the only public
   API at this point are the global `$localize` function and the two runtime
   helpers `loadTranslations()` and `clearTranslations()`.
   This does not mean that we will not expose additional helpers for 3rd
   party tooling in the future, but it avoid us preemptively exposing
   something that we might want to change in the near future.

Notes:

It is not possible to have the `$localize` code in the same Bazel package
as the rest of the code. If we did this, then the bundled `@angular/localize/init`
entry-point code contains all of the helper code, even though most of it is not used.

Equally it is not possible to have the `$localize` types (i.e. `LocalizeFn`
and `TranslateFn`) defined in the `@angular/localize/init` entry-point because
these types are needed for the runtime code, which is inside the primary
entry-point. Importing them from `@angular/localize/init` would run the
side-effect.

The solution is to have a Bazel sub-package at `//packages/localize/src/localize`
which contains these types and the `$localize` function implementation.
The primary `//packages/localize` entry-point imports the types without
any side-effect.
The secondary `//packages/localize/init` entry-point imports the `$localize`
function and attaches it to the global scope as a side-effect, without
bringing with it all the other utility functions.

BREAKING CHANGES:

The entry-points have changed:

* To attach the `$localize` function to the global scope import from
`@angular/localize/init`. Previously it was `@angular/localize`.

* To access the `loadTranslations()` and `clearTranslations()` functions,
import from `@angular/localize`. Previously it was `@angular/localize/run_time`.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-refactor-utils branch from b3e5f26 to 187bb1b Sep 12, 2019
Copy link
Member

left a comment

this looks great to me! thanks for addressing all the feedback! 👍

@kara kara closed this in 2bf5606 Sep 12, 2019
@petebacondarwin petebacondarwin deleted the petebacondarwin:i18n-refactor-utils branch Sep 13, 2019
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…2488)

This is a refactoring that moves the source code around to provide a better
platform for adding the compile-time inlining.

1. Move the global side-effect import from the primary entry-point to a
   secondary entry-point @angular/localize/init.

   This has two benefits: first it allows the top level entry-point to
   contain tree-shakable shareable code; second it gives the side-effect
   import more of an "action" oriented name, which indicates that importing
   it does something tangible

2. Move all the source code into the top src folder, and import the localize
   related functions into the localize/init/index.ts entry-point.

   This allows the different parts of the package to share code without
   a proliferation of secondary entry-points (i.e. localize/utils).

3. Avoid publicly exporting any utilities at this time - the only public
   API at this point are the global `$localize` function and the two runtime
   helpers `loadTranslations()` and `clearTranslations()`.
   This does not mean that we will not expose additional helpers for 3rd
   party tooling in the future, but it avoid us preemptively exposing
   something that we might want to change in the near future.

Notes:

It is not possible to have the `$localize` code in the same Bazel package
as the rest of the code. If we did this, then the bundled `@angular/localize/init`
entry-point code contains all of the helper code, even though most of it is not used.

Equally it is not possible to have the `$localize` types (i.e. `LocalizeFn`
and `TranslateFn`) defined in the `@angular/localize/init` entry-point because
these types are needed for the runtime code, which is inside the primary
entry-point. Importing them from `@angular/localize/init` would run the
side-effect.

The solution is to have a Bazel sub-package at `//packages/localize/src/localize`
which contains these types and the `$localize` function implementation.
The primary `//packages/localize` entry-point imports the types without
any side-effect.
The secondary `//packages/localize/init` entry-point imports the `$localize`
function and attaches it to the global scope as a side-effect, without
bringing with it all the other utility functions.

BREAKING CHANGES:

The entry-points have changed:

* To attach the `$localize` function to the global scope import from
`@angular/localize/init`. Previously it was `@angular/localize`.

* To access the `loadTranslations()` and `clearTranslations()` functions,
import from `@angular/localize`. Previously it was `@angular/localize/run_time`.

PR Close angular#32488
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Oct 14, 2019

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 Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.