Skip to content

feat(core): update TestBed to recognize Standalone Components#45809

Closed
AndrewKushnir wants to merge 1 commit intoangular:mainfrom
AndrewKushnir:tb_deps_overrides
Closed

feat(core): update TestBed to recognize Standalone Components#45809
AndrewKushnir wants to merge 1 commit intoangular:mainfrom
AndrewKushnir:tb_deps_overrides

Conversation

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Apr 29, 2022

This commit updates an internal logic of the TestBed to recognize Standalone Components to be able to apply the necessary overrides correctly.

PR Type

What kind of change does this PR introduce?

  • Feature

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added state: WIP area: core Issues related to the framework runtime target: major This PR is targeted for the next major release cross-cutting: standalone Issues related to the NgModule-less world labels Apr 29, 2022
@ngbot ngbot bot modified the milestone: Backlog Apr 29, 2022
@AndrewKushnir AndrewKushnir modified the milestones: Backlog, v14-candidates Apr 29, 2022
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Apr 29, 2022
Comment on lines 216 to 221
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkozlowski-opensource this is a use-case that might be tricky to support with Standalone Components (i.e. the TestBed.overrideProvider calls) without TestBed.configureTestingModule calls, since TestBed doesn't have enough information at the right time to make the necessary provider overrides. I think we can have a nice error message to let users know and suggest an alternative solution to use TestBed.overrideModule(ComponentDependenciesModule, {set: {providers: [ ... ]}}) instead, i.e. reference a type that declares that provider. So the test code will look like this:

// In this case the `TestBed.configureTestingModule ` is *not* required,
// TestBed knows where to find a class that declares that provider.
TestBed.overrideModule(
  ComponentDependenciesModule,
  {set: {providers: [ {provide: A, useValue: 'Overridden A' } ]}}
);

const fixture = TestBed.createComponent(MyStandaloneComp);

Choose a reason for hiding this comment

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

The real question for me here is this: should we be able to override providers of sth that got imported into a standalone module? Or should we rather consider it as a "black box" and allow people to override "imports" as the whole - but not pick into individual imports?

TBD

Copy link
Member

Choose a reason for hiding this comment

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

should we be able to override providers of sth that got imported into a standalone module?

I would say yes, since NgModules worked this way. You could override providers deep in dependencies. The override operation is an explicit piercing of any encapsulation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in cases when standalone types are used as dependencies (imported somewhere in an NgModule), but a non-standalone component is used to create a testing fixture (via TestBed.createComponent) - provider overrides would work correctly after this PR. However there are still sharp edges with provider overrides when a standalone component is used to create a fixture - I hope to improve this in a followup PR.

@AndrewKushnir AndrewKushnir marked this pull request as ready for review April 29, 2022 06:23
@pullapprove pullapprove bot removed the request for review from pkozlowski-opensource April 29, 2022 06:23
@AndrewKushnir
Copy link
Contributor Author

Presubmit.

@AndrewKushnir
Copy link
Contributor Author

Presubmit #2 (TGP).

This commit updates an internal logic of the TestBed to recognize Standalone Components to be able to apply the necessary overrides correctly.
Comment on lines 216 to 221
Copy link
Member

Choose a reason for hiding this comment

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

should we be able to override providers of sth that got imported into a standalone module?

I would say yes, since NgModules worked this way. You could override providers deep in dependencies. The override operation is an explicit piercing of any encapsulation.

const def = getComponentDef(moduleType);
const dependencies = maybeUnwrapFn(def.dependencies ?? []);
for (const dependency of dependencies) {
this.applyProviderOverridesToModule(dependency);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: should we rename this function to applyProviderOverridesToScope or something? Since it's not specifically targreting NgModules?

We can definitely do this in a cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I will create a followup cleanup PR 👍

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 3, 2022
@dylhunn
Copy link
Contributor

dylhunn commented May 3, 2022

This PR was merged into the repository by commit 401dec4.

@dylhunn dylhunn closed this in 401dec4 May 3, 2022
@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 Jun 3, 2022
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 area: core Issues related to the framework runtime cross-cutting: standalone Issues related to the NgModule-less world target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants