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

feat(core): support metadata reflection for native class types #22315

Closed
wants to merge 1 commit into from

Conversation

trotyl
Copy link
Contributor

@trotyl trotyl commented Feb 19, 2018

closes #21731

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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: #21731

What is the new behavior?

Using class inheritance in native class (targeting es2015) is now supported.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@trotyl
Copy link
Contributor Author

trotyl commented Feb 20, 2018

Timeout in unrelated spec, should be a flake.

@vicb vicb added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime labels Feb 20, 2018
@chuckjaz chuckjaz added target: patch This PR is targeted for the next patch release and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 21, 2018
@vicb vicb added the action: merge The PR is ready for merge by the caretaker label Feb 21, 2018
@vicb
Copy link
Contributor

vicb commented Feb 21, 2018

@vicb vicb closed this Feb 21, 2018
@vicb vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 21, 2018
@vicb vicb reopened this Feb 21, 2018
@vicb vicb removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 21, 2018
@vicb
Copy link
Contributor

vicb commented Feb 21, 2018

closing as rebased in 22356

@vicb vicb closed this Feb 21, 2018
@trotyl trotyl deleted the reflector-native-class branch February 22, 2018 01:04
// If we have no decorators, we only have function.length as metadata.
// In that case, to detect whether a child class declared an own constructor or not,
// we need to look inside of that constructor to check whether it is
// just calling the parent.
// This also helps to work around for https://github.com/Microsoft/TypeScript/issues/12439
// that sets 'design:paramtypes' to []
// if a class inherits from another class but has no ctor declared itself.
if (DELEGATE_CTOR.exec(type.toString())) {
if (DELEGATE_CTOR.exec(typeStr) ||
(INHERITED_CLASS.exec(typeStr) && !INHERITED_CLASS_WITH_CTOR.exec(typeStr))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please double check this logic ?

This change seems to trigger the following error:

class Base {
  constructor(t: Type) {...}
}

class Extend extends Base {
 // no ctor
}

// -> Can't resolve all parameters for BasicSearchResultChildComp: (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will have a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks weird, I can even confirm the OP's plunker works under 6.0.0-beta.5. Could you provide a little more information about:

  • Whether the code compiled to es5 or es2015+?
  • Does Base annotated with @Injectable()?
  • Is Type a class rather than a interface or type?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I can share for now:

---- controller.ts
// no annotation
export class Controller implements OnInit {
  @Input() result: ...;

  constructor(
      private readonly a: AType,
      private readonly g: GType) {}
// ...
}

---- comp.ts
@Component({
  selector: '...',
  templateUrl: ...,
})
export class Comp extends Controller {
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll investigate more tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be the lacking of class decorator in base class caused this problem, but I guess that's by design?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather call it a workaround. And IIRC it was working in some older versions without a decorator on an abstract base class.

Copy link
Contributor Author

@trotyl trotyl Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's due to lacking of inheritance support in some very old versions, the current behavior is the natural result of combining inheritance semantics and TypeScript metadata emitting rules.

If there's no way to fix (except changing the design), I think it can be called by design (but could be a design failure).

Also, this change only make the behavior consistent between compiled classes (functions) and native classes, previously the natives class doesn't follow inheritance semantics (cannot inherit parent parameters), so some errors are now revealed, not introduced (just better error message).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trotyl @devoto13 thanks for your help - the issue was on our side !
I'll probably reapply the change soon.

@vicb
Copy link
Contributor

vicb commented Feb 22, 2018

@trotyl Could you please add a test case for the error described in my previous comment.
I guess I'll have to revert this commit tomorrow (we did revert that change internally because it breaks some code)
Thanks.

@trotyl trotyl restored the reflector-native-class branch February 22, 2018 03:43
@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extended components do not receive constructor arguments when targetting ES6
5 participants