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

Proposal - Support change detection for Intl services. #5738

Closed
shlomiassaf opened this issue Jul 13, 2017 · 1 comment · Fixed by #5867
Closed

Proposal - Support change detection for Intl services. #5738

shlomiassaf opened this issue Jul 13, 2017 · 1 comment · Fixed by #5867
Assignees
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix needs: discussion Further discussion with the team is needed before proceeding

Comments

@shlomiassaf
Copy link
Contributor

Bug, feature request, or proposal: Proposal

What is the expected behavior?

Changing text in I18N (XXXIntl services) should reflect dynamically on components with OnPush change detection stratagy

What is the current behavior?

Intl services are simple classes, changing properties will not reflect in OnPush components unless they can be notified.

What is the use-case or motivation for changing an existing behavior?

Using libraries like ngx-translation or others that support dynamic, on the fly, language replacement..

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

All

Is there anything else we should know?

This feature can be supported in multiple ways.

It requires the Intl service to provide an emitter that fires events when a property changes.
The component using that service can listen to those changes and trigger CD.
This should probably go though a base Intl service that all Intl services extend.

There are 2 design aspect to resolve:

Emitting changes

The logic for emitting can be passive (transparent) or active (user initiated)
A Passive approach requires each property on the service to be a getter/setter and emit when set.
Active approach requires the user to run a method once the service is ready to update.

While the passive approach requires nothing from the user, it will require some boilerplate in the library and will emit on every property which isn't suitable for bulk changes although can be handled. The active approach requires an extra step but in most cases it is acceptable since this should happen once a language has changed.

Handling changes

This is an internal implementation in the components, which can be property specific or global.
Property specific means registering each property in a service as an observable and using the templates with async pipe to set the value. This approach comes with it's boilerplate and multiple subscriptions but it offers transparent CD integration and rx subscription management since everything is handled by angular. It also means that the services will have to export observables which is somewhat complex.

A global approach means subscribing to a service's change stream and acting upon it.
In practice it has much less boilerplate and it keeps the same template structure used today, it will only require registering to the change stream, run change detection on each hit and unsubscribing on destruction.

@jelbourn jelbourn added discussion feature This issue represents a new feature or feature request rather than a bug or bug fix labels Jul 14, 2017
@crisbeto crisbeto marked this as a duplicate of #5860 Jul 19, 2017
@crisbeto crisbeto self-assigned this Jul 19, 2017
crisbeto added a commit to crisbeto/material2 that referenced this issue Jul 19, 2017
Since we're switching all components to OnPush change detection, they won't necessarily react to dynamic changes in the i18n labels. These changes add an `EventEmitter` per provider that allow for components to react to those types of changes. Note that it's up to the consumer to call `provider.changes.emit()` in order to trigger the re-render.

Fixes angular#5738.
crisbeto added a commit to crisbeto/material2 that referenced this issue Jul 24, 2017
Since we're switching all components to OnPush change detection, they won't necessarily react to dynamic changes in the i18n labels. These changes add an `EventEmitter` per provider that allow for components to react to those types of changes. Note that it's up to the consumer to call `provider.changes.emit()` in order to trigger the re-render.

Fixes angular#5738.
andrewseguin pushed a commit that referenced this issue Jul 24, 2017
Since we're switching all components to OnPush change detection, they won't necessarily react to dynamic changes in the i18n labels. These changes add an `EventEmitter` per provider that allow for components to react to those types of changes. Note that it's up to the consumer to call `provider.changes.emit()` in order to trigger the re-render.

Fixes #5738.
andrewseguin pushed a commit that referenced this issue Jul 25, 2017
Since we're switching all components to OnPush change detection, they won't necessarily react to dynamic changes in the i18n labels. These changes add an `EventEmitter` per provider that allow for components to react to those types of changes. Note that it's up to the consumer to call `provider.changes.emit()` in order to trigger the re-render.

Fixes #5738.
crisbeto added a commit to crisbeto/material2 that referenced this issue Jul 26, 2017
Since we're switching all components to OnPush change detection, they won't necessarily react to dynamic changes in the i18n labels. These changes add an `EventEmitter` per provider that allow for components to react to those types of changes. Note that it's up to the consumer to call `provider.changes.emit()` in order to trigger the re-render.

Fixes angular#5738.
andrewseguin pushed a commit that referenced this issue Jul 27, 2017
Since we're switching all components to OnPush change detection, they won't necessarily react to dynamic changes in the i18n labels. These changes add an `EventEmitter` per provider that allow for components to react to those types of changes. Note that it's up to the consumer to call `provider.changes.emit()` in order to trigger the re-render.

Fixes #5738.
@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 6, 2019
@mmalerba mmalerba added the needs: discussion Further discussion with the team is needed before proceeding label Mar 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix needs: discussion Further discussion with the team is needed before proceeding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants