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(ivy): reorder provider type checks to align with VE #34433

Closed
wants to merge 2 commits into from

Conversation

@atscott
Copy link
Contributor

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 Dec 16, 2019
@atscott atscott requested a review from angular/fw-core as a code owner Dec 16, 2019
@googlebot googlebot added the cla: yes label Dec 16, 2019
@alxhub
alxhub approved these changes Dec 16, 2019
Copy link
Contributor

kara left a comment

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

atscott added 2 commits 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 force-pushed the atscott:usefactory branch from 62c5236 to ef834eb Dec 16, 2019
@atscott

This comment has been minimized.

Copy link
Contributor Author

atscott commented Dec 16, 2019

@kara
kara approved these changes Dec 16, 2019
Copy link
Contributor

kara left a comment

LGTM, thanks!

kara added 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

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 16, 2020

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.
Projects
None yet
4 participants
You can’t perform that action at this time.