-
Notifications
You must be signed in to change notification settings - Fork 26.2k
Refactor ReflectionCapabilities to rely on metadata for identifying 'pass-through' constructors #22642
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
Refactor ReflectionCapabilities to rely on metadata for identifying 'pass-through' constructors #22642
Conversation
This change was an attempt to improve the way in which angular detects "implicit" constructor functions. Unfortunately I've hit a wall with respect to the edge case I describe above, where the new routine incorrectly determines the parameters of the @Injectable()
class BaseClass {
constructor (public data1: string, public data2: number) {
}
}
class Foo extends BaseClass {
constructor () {
super('hello');
}
} I'm issuing this PR just to request feedback on the approach - perhaps someone else can see a way to make this approach work? Ideally we could have a check to determine if the first constructor function in the chain is |
I've made some changes to try and work around the edge case described above - specifically I've added the It does this by checking that the type constructor...
Only one change to the tests had to be made to mark up a test subclass with an |
Because fixing the problem here is non-trivial I've also opened an issue on ng-packagr around its codegen: ng-packagr/ng-packagr#679 |
@deadalusai Thanks for the fix. Can you force the ci/circleci to re-run, I think it may be flake. I think this can be merged after v6.0.0 is out. |
…entifying 'pass-through' constructors - avoid the regex check whenever possible - generate default metadata only after we've checked all parent constructors - add _isInjectable to and early if a type is not "injectable"
Also closes #13211 |
Fixes that the table does not render properly when used inside of a ES2015 application. This is due to an ongoing Angular issue which has been caused due to a brittle Regex check [introduced here](angular/angular#22356 (comment)) that should be replaced with a more clean approach for identifying classes that inherit metadata from another class. * angular/angular#22642 * angular/tsickle#760 Fixes angular#9329
Fixes that some components does not render properly when used inside of a ES2015 JIT application. This is due to an ongoing Angular issue which has been caused due to a brittle Regex check [introduced here](angular/angular#22356 (comment)) that should be replaced with a more clean approach for identifying classes that inherit metadata from another class. * angular/angular#22642 * angular/tsickle#760 See the more detailed issue here: angular#12760 Fixes angular#9329.
Fixes that some components does not render properly when used inside of a ES2015 JIT application. This is due to an ongoing Angular issue which has been caused due to a brittle Regex check [introduced here](angular/angular#22356 (comment)) that should be replaced with a more clean approach for identifying classes that inherit metadata from another class. * angular/angular#22642 * angular/tsickle#760 See the more detailed issue here: #12760 Fixes #9329.
Fixes that some components does not render properly when used inside of a ES2015 JIT application. This is due to an ongoing Angular issue which has been caused due to a brittle Regex check [introduced here](angular/angular#22356 (comment)) that should be replaced with a more clean approach for identifying classes that inherit metadata from another class. * angular/angular#22642 * angular/tsickle#760 See the more detailed issue here: #12760 Fixes #9329.
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 (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
Closing as abandoned |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Refactor
ReflectionCapabilities
to rely on metadata for identifying 'pass-through' constructorsPR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
This is an effort to improve the way
ReflectionCapabilities
determines if a constructor function is a "pass-through" or implicit constructor and decide when we should take constructor parameters from the parent type.For example:
...will generate a
Foo
constructor like:The current implementation works by running a regular expression against the generated javascript code to guess whether the function calls the super constructor in the manner above. The current regex does not work if the generated code is a little more complicated, like:
What is the new behavior?
Rather than try to make this regex cover all possible permutations of this pattern, can we instead determine if a constructor is implicit by inspecting metadata alone?
This change relies on the following:
tsc
does not emitdesign:paramtypes
metadata for types with implicit constructorstsickle
ensures the same for tsickle code: Only emitctorParameters
when the class has a constructor tsickle#760If a constructor function does not provide constructor parameter metadata and reports no explicit arguments, then we attempt to inherit constructor parameters from the parent type.
Does this PR introduce a breaking change?
Other information
Edit: I have a work around for this issue described here in the form of the
_isInjectable
check, which tries to determine if a type has been prepared for injection in some way before trying to determine the parameter list.This change works for all code currently in the
./test.sh node
test suite, though it may be working "accidentally" for some types. This is because it does not work correctly with the following pattern:Specifically, attempting to inject a non-attributed class which inherits from an attributed class.
Because the class
Foo
does not have any attributes, it does not get any constructor metadata and so looks like it could be a "pass-through" constructor. The super class does have constructor metadata,so the routine incorrectly uses that parameter list.
Foo
is technically incorrectly declared - it should have the@Injectable()
attribute, however because it does not we determine that the parameter set forFoo
is[string]
and the code eventually fails with the following unhelpful error: