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

fix(stepper): intl provider not being picked up in lazy-loaded modules #12934

Merged
merged 1 commit into from
Oct 19, 2018

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Sep 1, 2018

Along the same lines as #7988 and #7895. Fixes the consumer-provided MatStepperIntl instance not being picked up inside lazy-loaded modules.

Fixes #12904.

Along the same lines as angular#7988 and angular#7895. Fixes the consumer-provided `MatStepperIntl` instance not being picked up inside lazy-loaded modules.

Fixes angular#12904.
@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Sep 1, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 1, 2018
@@ -54,6 +54,6 @@ import {MatStepperIntl} from './stepper-intl';
MatStepHeader,
MatStepperIcon,
],
providers: [MatStepperIntl, ErrorStateMatcher],
providers: [MAT_STEPPER_INTL_PROVIDER, ErrorStateMatcher],
Copy link
Member Author

Choose a reason for hiding this comment

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

@jelbourn I would've assumed that this was already taken care of by using providedIn: 'root', but based on the discussion in #12904 that doesn't seem to be the case. I wonder whether it isn't breaking down, because we were declaring the provider in here.


/** @docs-private */
export function MAT_STEPPER_INTL_PROVIDER_FACTORY(parentIntl: MatStepperIntl) {
return parentIntl || new MatStepperIntl();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to do this; it makes it impossible to re-provide the intl config for a subset of the app. I would have thought that including the provide: for the custom implementation in the lazy-loaded module would bring in the customization.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't look like that's the case, based on our discussion in the issue. FWIW we do the same as here for the sort header and paginator.

Copy link
Member

Choose a reason for hiding this comment

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

In the issue it said that they were providing their custom intl implementation in the root module and not in the lazy-loaded module, which I think is the problem. Using providedIn isn't an option here because they have to re-provide the existing token.

Copy link
Member Author

Choose a reason for hiding this comment

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

That means that they'd have to remember to add the provider for each new lazy-loaded module that is introduced. I don't think that a translation provider is something you'd generally want to be able to re-provide for different sections of the app. Also if we went with the providedIn approach, how would we handle the other similar providers that already use the old approach?

Copy link
Member

Choose a reason for hiding this comment

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

I don't follow what you mean in the second part about providedIn

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean that if we went with the approach that is currently in master (having a providedIn and having people provide it themselves in the lazy-loaded modules), it makes the stepper's i18n provider inconsistent with the other i18n providers (like sort header or paginator) which are provided in the same way as this PR.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Oct 4, 2018
@josephperrott josephperrott merged commit 675aa66 into angular:master Oct 19, 2018
@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: MatStepperIntl custom implementation not injected
5 participants