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

fix(ivy): ngcc - recognize synthesized constructors #27897

Closed
wants to merge 8 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Jan 2, 2019

ngcc needs to be able to identify synthesized constructors in order to let ngtsc create a base factory call, instead of assuming a no-args constructor.

This is important for Material, e.g. the following code (source):

export class MatTable<T> extends CdkTable<T> {
  /** Overrides the sticky CSS class set by the `CdkTable`. */
  protected stickyCssClass = 'mat-table-sticky';
}

is compiled to ES2015 (source):

class MatTable extends CdkTable {
    constructor() {
        super(...arguments);
        /**
         * Overrides the sticky CSS class set by the `CdkTable`.
         */
        this.stickyCssClass = 'mat-table-sticky';
    }
}

and downleveled to ES5 (source):

var MatTable = /** @class */ (function (_super) {
    __extends(MatTable, _super);
    function MatTable() {
        var _this = _super !== null && _super.apply(this, arguments) || this;
        /**
         * Overrides the sticky CSS class set by the `CdkTable`.
         */
        _this.stickyCssClass = 'mat-table-sticky';
        return _this;
    }
    MatTable.decorators = [/* ... */];
    return MatTable;
}(CdkTable));

Currently, ngcc reports the MatTable class to have a no-args constructor, resulting in a factory function that doesn't inject the dependencies of CdkTable.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • 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:

@JoostK JoostK changed the title [WIP] fix(ivy): ngcc - recognize synthesized constructors in ES5 code [WIP] fix(ivy): ngcc - recognize synthesized constructors Jan 3, 2019
@JoostK
Copy link
Member Author

JoostK commented Jan 3, 2019

@petebacondarwin I have updated the code with some more documentation and added ES2015 support. For the ES2015 tests I choose to include them in the existing file, whereas I split out the ES5 constructor tests into a separate file. I am now wondering which is better 🤔

Regarding the tests, I feel it's a bit overkill to add tests that verify all possible mismatches in AST structure. How do you feel about that? Are the current tests sufficient or do you see some additional ones that should be added?

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM

@petebacondarwin
Copy link
Member

I would be tempted to keep the tests in the files rather than splitting them out. It makes the git history a bit simpler.

@JoostK
Copy link
Member Author

JoostK commented Jan 4, 2019

I added an integration test in b29dd5c only to find that ES5 didn't actually work :-)

I added fixup commit 2c9df97 in which hasBaseClass is specialized for ES5, that should fix the issue.

The test still fails though, as we're now emitting const into an ES5 file. This is mostly due to the same issue as we recently had with emitting ! into JavaScript files. There's a few options to solve this:

  1. Always translate declarations as var instead of const.
  2. Let the transformer accept a mode such that it can decide how to emit a declaration.
  3. Avoid the Angular to TS translation and emit from the Angular AST.

EDIT: Discussed offline that emitting const in ES5 should not cause any issues.

@JoostK JoostK changed the title [WIP] fix(ivy): ngcc - recognize synthesized constructors fix(ivy): ngcc - recognize synthesized constructors Jan 4, 2019
@kara kara added the comp: ivy label Jan 4, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jan 4, 2019
@mhevery mhevery added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Jan 8, 2019
@mhevery
Copy link
Contributor

mhevery commented Jan 8, 2019

@kara
Copy link
Contributor

kara commented Jan 9, 2019

@JoostK Can you rebase this one? (ping me when you're ready and I'll merge)

@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jan 9, 2019
@JoostK
Copy link
Member Author

JoostK commented Jan 9, 2019

@kara I can't at the moment, I'm on holiday this week. Will rebase coming weekend if none of you have beaten me to it (the conflict is trivial to resolve, only a Js docblock was added)

A constructor function may have been "synthesized" by TypeScript during
JavaScript emit, in the case no user-defined constructor exists and e.g.
property initializers are used. Those initializers need to be emitted
into a constructor in JavaScript, so the TypeScript compiler generates a
synthetic constructor.

This commit adds identification of such constructors as ngcc needs to be
able to tell if a class did originally have a constructor in the
TypeScript source. When a class has a superclass, a synthesized
constructor must not be considered as a user-defined constructor as that
prevents a base factory call from being created by ngtsc, resulting in a
factory function that does not inject the dependencies of the superclass.
Hence, we identify a default synthesized super call in the constructor
body, according to the structure that TypeScript emits.
@petebacondarwin
Copy link
Member

I have rebased on master. This has triggered the code-owners approval process.

@kara kara unassigned JoostK Jan 9, 2019
@kara kara added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 9, 2019
@JoostK JoostK deleted the ngcc-reflect-constructors branch January 13, 2019 13:05
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
A constructor function may have been "synthesized" by TypeScript during
JavaScript emit, in the case no user-defined constructor exists and e.g.
property initializers are used. Those initializers need to be emitted
into a constructor in JavaScript, so the TypeScript compiler generates a
synthetic constructor.

This commit adds identification of such constructors as ngcc needs to be
able to tell if a class did originally have a constructor in the
TypeScript source. When a class has a superclass, a synthesized
constructor must not be considered as a user-defined constructor as that
prevents a base factory call from being created by ngtsc, resulting in a
factory function that does not inject the dependencies of the superclass.
Hence, we identify a default synthesized super call in the constructor
body, according to the structure that TypeScript emits.

PR Close angular#27897
@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
action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants