Skip to content

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Dec 16, 2019

The orderering matters because we don't currently throw if multiple
configurations are provided (i.e. provider has both useExisting and
useFactory). We should actually throw an error in this case, but to
avoid another breaking change in v9, this PR simply aligns the Ivy
behavior with ViewEngine.

current Ivy:

} else if (isExistingProvider(provider)) {
factory = () => ɵɵinject(resolveForwardRef(provider.useExisting));
} else if (isFactoryProvider(provider)) {
factory = () => provider.useFactory(...injectArgs(provider.deps || []));

VE:
} else if ((provider as FactoryProvider).useFactory) {
fn = (provider as FactoryProvider).useFactory;
} else if ((provider as ExistingProvider).useExisting) {
// Just use IDENT

@atscott atscott requested a review from alxhub December 16, 2019 18:20
@atscott atscott requested a review from a team as a code owner December 16, 2019 18:20
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Can you please add a test that verifies this behavior with a comment that links to the Jira?

Also - there's a typo in the commit message: orderering => ordering

@kara kara added area: core Issues related to the framework runtime comp: ivy action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Dec 16, 2019
@ngbot ngbot bot modified the milestone: needsTriage Dec 16, 2019
The ordering matters because we don't currently throw if multiple
configurations are provided (i.e. provider has *both* useExisting and
useFactory). We should actually throw an error in this case, but to
avoid another breaking change in v9, this PR simply aligns the Ivy
behavior with ViewEngine.
@atscott atscott removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 16, 2019
@atscott
Copy link
Contributor Author

atscott commented Dec 16, 2019

VE presubmit
Ivy presubmit

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kara kara added the action: presubmit The PR is in need of a google3 presubmit label Dec 16, 2019
@atscott atscott added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Dec 16, 2019
kara pushed a commit that referenced this pull request Dec 16, 2019
The ordering matters because we don't currently throw if multiple
configurations are provided (i.e. provider has *both* useExisting and
useFactory). We should actually throw an error in this case, but to
avoid another breaking change in v9, this PR simply aligns the Ivy
behavior with ViewEngine.

PR Close #34433
@kara kara closed this in 357a073 Dec 16, 2019
@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 Jan 16, 2020
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 cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants