Skip to content
This repository was archived by the owner on Oct 29, 2023. It is now read-only.

demonstrate runtime error for @nebular/theme #526

Merged
merged 2 commits into from
Nov 8, 2019
Merged

demonstrate runtime error for @nebular/theme #526

merged 2 commits into from
Nov 8, 2019

Conversation

rayman1104
Copy link
Contributor

@rayman1104 rayman1104 commented Nov 7, 2019

Repro for #529

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@rayman1104
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@mgechev mgechev merged commit 1aeae93 into angular:master Nov 8, 2019
@rayman1104 rayman1104 deleted the nebular_theme branch November 9, 2019 11:54
@petebacondarwin
Copy link
Contributor

The error is that the injector is not able to create NbScrollStrategyOptions. This is because it is not finding the injectable definition for this type.

I can see that ngcc is doing the right thing:

    NbScrollStrategyOptions.ngInjectableDef = ɵɵdefineInjectable({ factory: function NbScrollStrategyOptions_Factory() { return new NbScrollStrategyOptions(ɵɵinject(NbLayoutScrollService), ɵɵinject(ScrollDispatcher$1), ɵɵinject(NbViewportRulerAdapter), ɵɵinject(NgZone), ɵɵinject(NB_DOCUMENT)); }, token: NbScrollStrategyOptions, providedIn: "root" });

The problem is that getInjectableDef() is not doing the right thing... See https://github.com/angular/angular/blob/86104b82b8af87efab46db50962e6b16f3724672/packages/core/src/di/interface/defs.ts#L192-L202

The code is effectively:

export function getInjectableDef<T>(type: any): ɵɵInjectableDef<T>|null {
  const def = (type.ɵprov || type.ngInjectableDef) as ɵɵInjectableDef<T>;
  return def && def.token === type ? def : null;
}

Checking the def.token === type is to prevent bringing in the definition from the type's super class.

But in this case it is wrong because, while ɵprov is inherited and so should not be used ngInjectableDef is available and not inherited. So that one should be used.

@petebacondarwin
Copy link
Contributor

Actually I just realised that ngcc should be using the "new" name (i.e. ɵprov replaced ngInjectableDef). So perhaps there are two fixes...

  1. make core more resilient to a derived class using the old name when a super class uses the new name
  2. fix ngcc to output the new name rather than the old name

@petebacondarwin
Copy link
Contributor

Actually!!

It turns out that this ngInjectableDef was already in the library published to npm, so ngcc had no hand in defining it. I checked and ngcc will always add the correct ɵprov property. So it is only legacy libraries that have this problem.

Therefore the fix really is only 1) from above.

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Nov 12, 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
kara pushed a commit to angular/angular 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
kara pushed a commit to angular/angular 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants