Skip to content

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

Closed
wants to merge 1 commit into from
Closed

Refactor ReflectionCapabilities to rely on metadata for identifying 'pass-through' constructors #22642

wants to merge 1 commit into from

Conversation

deadalusai
Copy link

@deadalusai deadalusai commented Mar 8, 2018

Refactor ReflectionCapabilities to rely on metadata for identifying 'pass-through' constructors

  • avoid the regex check whenever possible
  • generate default metadata only after we've checked all parent constructors

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

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:

class Foo extends Base {
}

...will generate a Foo constructor like:

function Foo () {
    __super.apply(this, arguments);
}

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:

function FooComponent () {
    __super.apply(this, __tslib.__spread(arguments));
}

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:

If 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?

[X] Yes
[ ] No

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:

@Injectable()
class BaseClass {
    constructor (public data: string) {
    }
}

// NOTE: no attribute to trigger metadata generation
class Foo extends BaseClass {
    constructor () {
        super('hello');
    }
}

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 for Foo is [string] and the code eventually fails with the following unhelpful error:

NullInjectorError: No provider for String!

@deadalusai
Copy link
Author

deadalusai commented Mar 8, 2018

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 Foo type in this code to be string, number.

@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 @Injectable (or otherwise annotated) and bail early if it is not. This is easy to do with the tsickle codegen however I don't think enough metadata is available for tsc generated code.

@deadalusai
Copy link
Author

I've made some changes to try and work around the edge case described above - specifically I've added 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.

It does this by checking that the type constructor...

  • has es5 attributes/parameters properties, or
  • has tsickle properties, or
  • has been marked up with an Angular attribute (@Injectable, @Component etc)

Only one change to the tests had to be made to mark up a test subclass with an @ClassDecorator attribute (simulating @Injectable).

@alexeagle alexeagle added the area: core Issues related to the framework runtime label Mar 8, 2018
@deadalusai
Copy link
Author

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

@mhevery mhevery self-requested a review March 21, 2018 17:13
@mhevery mhevery added the target: patch This PR is targeted for the next patch release label Mar 21, 2018
@mhevery
Copy link
Contributor

mhevery commented Mar 21, 2018

@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"
@trotyl
Copy link
Contributor

trotyl commented Jul 27, 2018

Also closes #13211

devversion added a commit to devversion/material2 that referenced this pull request Aug 20, 2018
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
devversion added a commit to devversion/material2 that referenced this pull request Aug 20, 2018
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.
jelbourn pushed a commit to angular/components that referenced this pull request Aug 21, 2018
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.
jelbourn pushed a commit to angular/components that referenced this pull request Aug 29, 2018
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.
@deadalusai
Copy link
Author

@trotyl @mhevery do you consider this PR still relevant and viable? I no longer have an environment available to easily re-test this code with the latest master, but I'm willing to build out an environment if need be.

@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 (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@deadalusai
Copy link
Author

Closing as abandoned

@angular-automatic-lock-bot
Copy link

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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime cla: no target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants