Skip to content

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

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.

Sign up for GitHub

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

Merged
josephperrott merged 1 commit into from
Oct 19, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/lib/stepper/stepper-intl.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Injectable} from '@angular/core';
import {Injectable, Optional, SkipSelf} from '@angular/core';
import {Subject} from 'rxjs';


Expand All@@ -22,3 +22,16 @@ export class MatStepperIntl {
/** Label that is rendered below optional steps. */
optionalLabel: string = 'Optional';
}


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

@jelbournjelbournSep 4, 2018

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

@crisbetocrisbetoSep 4, 2018

Copy link
Copy Markdown
MemberAuthor

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.

@jelbournjelbournSep 4, 2018

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.

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.

@crisbetocrisbetoSep 4, 2018

Copy link
Copy Markdown
MemberAuthor

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?

@jelbournjelbournSep 4, 2018

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 don't follow what you mean in the second part about providedIn

@crisbetocrisbetoSep 5, 2018

Copy link
Copy Markdown
MemberAuthor

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.

}

/** @docs-private */
export const MAT_STEPPER_INTL_PROVIDER = {
provide: MatStepperIntl,
deps: [[new Optional(), new SkipSelf(), MatStepperIntl]],
useFactory: MAT_STEPPER_INTL_PROVIDER_FACTORY
};
4 changes: 2 additions & 2 deletions src/lib/stepper/stepper-module.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -18,7 +18,7 @@ import {MatStepLabel} from './step-label';
import {MatHorizontalStepper, MatStep, MatStepper, MatVerticalStepper} from './stepper';
import {MatStepperNext, MatStepperPrevious} from './stepper-button';
import {MatStepperIcon} from './stepper-icon';
import {MatStepperIntl} from './stepper-intl';
import {MAT_STEPPER_INTL_PROVIDER} from './stepper-intl';


@NgModule({
Expand DownExpand Up@@ -54,6 +54,6 @@ import {MatStepperIntl} from './stepper-intl';
MatStepHeader,
MatStepperIcon,
],
providers: [MatStepperIntl, ErrorStateMatcher],
providers: [MAT_STEPPER_INTL_PROVIDER, ErrorStateMatcher],

@crisbetocrisbetoSep 1, 2018

Copy link
Copy Markdown
MemberAuthor

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.

})
export class MatStepperModule {}