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(core): support `ngInjectableDef` on types with inherited `ɵprov` #33732

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Nov 11, 2019

The ngInjectableDef property was renamed to ɵprov, but core must
still support both because there are published libraries that use the
older term.

We are only interested in such properties that are defined directly on
the type being injected, not on base classes. So there is a check that
the defintion is specifically for the given type.

Previously if you tried to inject a class that had ngInjectableDef but
also inherited ɵprov then the check would fail on the ɵprov property
and never even try the ngInjectableDef property resulting in a failed
injection.

This commit fixes this by attempting to find each of the properties
independently.

Fixes angular/ngcc-validation#526

@petebacondarwin petebacondarwin requested review from angular/fw-compiler as code owners Nov 11, 2019
@googlebot googlebot added the cla: yes label Nov 11, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 11, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:core-getInjectableDef-fix branch from 0a803aa to 1789f6b Nov 11, 2019
@alxhub
alxhub approved these changes Nov 11, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Nov 11, 2019

@kara
kara approved these changes Nov 11, 2019
Copy link
Contributor

kara left a comment

LGTM

The `ngInjectableDef` property was renamed to `ɵprov`, but core must
still support both because there are published libraries that use the
older term.

We are only interested in such properties that are defined directly on
the type being injected, not on base classes. So there is a check that
the defintion is specifically for the given type.

Previously if you tried to inject a class that had `ngInjectableDef` but
also inherited `ɵprov` then the check would fail on the `ɵprov` property
and never even try the `ngInjectableDef` property resulting in a failed
injection.

This commit fixes this by attempting to find each of the properties
independently.

Fixes angular/ngcc-validation#526
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:core-getInjectableDef-fix branch from 1789f6b to 941bd9c Nov 12, 2019
@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 12, 2019

@kara kara closed this in 641c671 Nov 12, 2019
kara added a commit that referenced this pull request Nov 12, 2019
…33732)

The `ngInjectableDef` property was renamed to `ɵprov`, but core must
still support both because there are published libraries that use the
older term.

We are only interested in such properties that are defined directly on
the type being injected, not on base classes. So there is a check that
the defintion is specifically for the given type.

Previously if you tried to inject a class that had `ngInjectableDef` but
also inherited `ɵprov` then the check would fail on the `ɵprov` property
and never even try the `ngInjectableDef` property resulting in a failed
injection.

This commit fixes this by attempting to find each of the properties
independently.

Fixes angular/ngcc-validation#526

PR Close #33732
@petebacondarwin petebacondarwin deleted the petebacondarwin:core-getInjectableDef-fix branch Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.