fix(compiler): use base class parameters for any non-decorated class … #44092
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is an alternative to #37811 and greatly simplifies things. |
JoostK
force-pushed
the
remove-delegate-ctor-logic
branch
2 times, most recently
from
b7dcede
to
233ca9a
…without constructor parameters Previously, the JIT compiler would take into consideration whether a type had a constructor function that appeared to have been synthesized. For synthesized constructors, the JIT compiler would fall back to using the parameter metadata from the base class, if any. However, the detection of synthesized constructors was rather error-prone: it required complex regexes to support various syntax forms, i.e. ES2015 vs ES5 code, minified vs non-minified code, TypeScript versus Rollup generated constructors etc. The primary driver for this change has been the inability of these regexes to deal with constructors that have been instrumented with code coverage reporting logic. This logic affects the syntax forms of constructors, which threw off the JIT compiler in no longer considering these constructors as synthesized. Consequently, the compiler would not fall back to using the base type's parameters but would generate a factory function without any dependencies. At runtime, this causes instances created using these factories to be missing their dependencies. The detection of synthesized constructors used to be necessary back in the day, when Angular supported classes without decorators to take part in DI. This is no longer allowed since Angular 10, as any class that takes part in Angular's DI mechanism is required to have a decorator. When a decorator is present, the JIT compiler is able to use the decorator metadata to derive whether a constructor was originally present. Additionally, microsoft/TypeScript#12439 used to be a blocking issue for which manual detection of synthesized constructors was a necessity. This issue has long been solved so is no longer relevant today. This commit removes all logic to detect synthesized constructors altogether, per the above observation that decorators must be used for DI to work correctly. Hence, when no metadata is present and the constructor function does not have any parameters, we now conclude that the type does not have its own constructor and fall back to the base type. This resolves the limitations with the regexes altogether. Fixes angular#15212 Fixes angular#31337
JoostK
reviewed
Nov 6, 2021
| @@ -40,18 +40,21 @@ class TestInterceptor implements HttpInterceptor { | |||
| } | |||
| } | |||
|
|
|||
| @Injectable() | |||
There was a problem hiding this comment.
These changes show the risk involved with this change; we may need to consider this a breaking change if we go ahead with it.
The primary question is whether a class with its own, empty constructor is a valid candidate for usage with useClass providers even if it does not have a decorator. It used to be, but will no longer be with the changes in this PR.
|
Closing this in favor of #44732, as this change is too breaking. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
…without constructor parameters
Previously, the JIT compiler would take into consideration whether a
type had a constructor function that appeared to have been synthesized.
For synthesized constructors, the JIT compiler would fall back to using
the parameter metadata from the base class, if any. However, the
detection of synthesized constructors was rather error-prone: it
required complex regexes to support various syntax forms, i.e. ES2015 vs
ES5 code, minified vs non-minified code, TypeScript versus Rollup
generated constructors etc.
The primary driver for this change has been the inability of these
regexes to deal with constructors that have been instrumented with code
coverage reporting logic. This logic affects the syntax forms of
constructors, which threw off the JIT compiler in no longer considering
these constructors as synthesized. Consequently, the compiler would not
fall back to using the base type's parameters but would generate a
factory function without any dependencies. At runtime, this causes
instances created using these factories to be missing their
dependencies.
The detection of synthesized constructors used to be necessary back in
the day, when Angular supported classes without decorators to take part
in DI. This is no longer allowed since Angular 10, as any class that
takes part in Angular's DI mechanism is required to have a decorator.
When a decorator is present, the JIT compiler is able to use the
decorator metadata to derive whether a constructor was originally
present. Additionally, microsoft/TypeScript#12439 used to be a blocking
issue for which manual detection of synthesized constructors was a
necessity. This issue has long been solved so is no longer relevant
today.
This commit removes all logic to detect synthesized constructors
altogether, per the above observation that decorators must be used for
DI to work correctly. Hence, when no metadata is present and the
constructor function does not have any parameters, we now conclude that
the type does not have its own constructor and fall back to the base
type. This resolves the limitations with the regexes altogether.
Fixes #15212
Fixes #31337
The text was updated successfully, but these errors were encountered: