-
Notifications
You must be signed in to change notification settings - Fork 26.2k
feat(core): support metadata reflection for native class types #22315
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
Conversation
b15ce3a
to
73f6710
Compare
Timeout in unrelated spec, should be a flake. |
closing as rebased in 22356 |
// 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))) { |
There was a problem hiding this comment.
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: (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have a look
There was a problem hiding this comment.
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
ores2015+
? - Does
Base
annotated with@Injectable()
? - Is
Type
a class rather than a interface or type?
There was a problem hiding this comment.
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 {
}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trotyl Could you please add a test case for the error described in my previous comment. |
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. |
closes #21731
PR 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: #21731
What is the new behavior?
Using class inheritance in native class (targeting
es2015
) is now supported.Does this PR introduce a breaking change?
Other information