feat(ivy): initial ngcc code dump by petebacondarwin · Pull Request #24897 · angular/angular

petebacondarwin · 2018-07-16T09:31:44Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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)
[x] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

Does this PR introduce a breaking change?

[ ] Yes
[x] No

googlebot · 2018-07-16T09:31:47Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

petebacondarwin · 2018-07-16T09:35:04Z


  getAllImports(contextPath: string, rewriteCoreImportsTo: ts.SourceFile|null):
-      {name: string, as: string}[] {
+    { name: string, as: string }[] {


Oops I guess I have a different curly brace formatting approach!

petebacondarwin · 2018-07-16T09:37:03Z

+      }
+      return `Error: ${message}`;
+    });
+    throw new Error(`Typescript diagnostics failed! ${errors.join(', ')}`);


This block of changes just makes the error output a bit clearer - and is in keeping with how the TS compiler outputs errors.

petebacondarwin · 2018-07-16T09:37:25Z

      return undefined;
    }
-    return ts.createSourceFile(fileName, contents, languageVersion, undefined, ts.ScriptKind.TS);
+    return ts.createSourceFile(fileName, contents, languageVersion);


Not all source files are TypeScript :-P

mary-poppins · 2018-07-16T13:29:49Z

You can preview ee37256 at https://pr24897-ee37256.ngbuilds.io/.

alxhub

Initial round of comments. Looks great! Should be only minor stylistic updates.

alxhub · 2018-07-17T06:42:56Z

 function isFunctionOrMethodDeclaration(node: ts.Node): node is ts.FunctionDeclaration|
    ts.MethodDeclaration {
-  return ts.isFunctionDeclaration(node) || ts.isMethodDeclaration(node);
+  return ts.isFunctionDeclaration(node) || ts.isMethodDeclaration(node) ||


Add ts.FunctionExpression to the type discriminator signature.

alxhub · 2018-07-17T06:47:45Z

+
+export function getFakeCore() {
+  return {
+    name: 'node_modules/@angular/core/index.ts',


Can you add a TODO here to unify this with the actual fake core library - I'd love to not have this duplicated.

alxhub · 2018-07-17T06:48:43Z

+import * as ts from 'typescript';
+
+import {ClassMember, ClassMemberKind, Decorator, Parameter} from '../../../ngtsc/host';
+import {TypeScriptReflectionHost, reflectObjectLiteral} from '../../../ngtsc/metadata/src/reflector';


Import only from ../../../ngtsc/metadata - if the types you need aren't re-exported through the index.ts there, then add them.

alxhub · 2018-07-17T06:50:51Z

+   * `null` if either no decorators were present or if the declaration is not of a decoratable type.
+   */
+  getDecoratorsOfDeclaration(declaration: ts.Declaration): Decorator[]|null {
+    // QUESTION: do we need to consider decoratored functions (i.e. not ES5 class constructors)?


Yes, you can have e.g:

@HostListener('click') onClick(): void {...} @HostBinding('class.foo') get isClassFoo(): boolean { ... }

@alxhub - actually in practice this method is only called with a non-class from within methods on TypeScriptReflectionHost as an implementation detail. For instance inside getConstructorParameters() and _reflectMember(). All of these calls are in methods that are not called in the ESM2015 and ESM5 reflection host classes.

Moreover in ES2015 and ES5 there are occasions where a decorated property does not even have a declaration, so this method would not be applicable anyway.

Therefore I don't believe that this should be a requirement of the ReflectionHost interface.

Instead, can we rename this interface method to getDecoratorsOfClass(declaration: ts.Declaration) and then refactor the TypeScriptReflectionHost to have a protected method getDecoratorsOfDeclaration that can find decorators on any declaration, which is what is used internally for getDecoratorsOfClass and those methods mentioned above.

Ah, understood and agreed. getDecoratorsOfClass makes more sense.

alxhub · 2018-07-17T06:52:28Z

+        if (decoratorsIdentifier && decoratorsIdentifier.parent) {
+          // AST of the array of decorator values
+          const decoratorsArray =
+              (decoratorsIdentifier.parent as ts.AssignmentExpression<ts.EqualsToken>).right;


I generally prefer to use the type guards and throw if something doesn't match, rather than cast. It's more verbose, but it helps catch edge cases that we didn't consider, especially when the TypeScript version updates and semantics change slightly.

alxhub · 2018-07-17T07:02:16Z

+   * So we need to dig around inside to get hold of the "class" symbol.
+   * @param declaration the top level declaration that represents an exported class.
+   */
+  getClassSymbol(declaration: ts.Declaration) {


Declare the return type.

alxhub · 2018-07-17T07:02:28Z

+   * In ESM5 there is no "class" so the constructor that we want is actually the declaration
+   * function itself.
+   */
+  protected getConstructorParameterDeclarations(classSymbol: ts.Symbol) {


Declare return type, and throughout.

alxhub · 2018-07-17T07:03:25Z

+   * ```
+   */
+  protected getConstructorDecorators(classSymbol: ts.Symbol) {
+    if (classSymbol.exports && classSymbol.exports.has(CONSTRUCTOR_PARAMS)) {


Consider inverting the logic here, with early returns if the preconditions aren't met, instead of nested if statements.

Actually I am sorely tempted to implement tsquery for a lot of this logic. It would be cleaner to read and easier to maintain.

alxhub · 2018-07-17T07:07:37Z

+                const decorators = this.host.getDecoratorsOfDeclaration(declaration);
+                if (decorators) {
+                  return new ParsedClass(declaration.name !.text, declaration, decorators);
+                }


Explicit return

alxhub · 2018-07-17T07:09:06Z

+
+  // Add the imports at the top of the file
+  addImports(output: MagicString, imports: {name: string; as: string;}[]): void {
+    // QUESTION: Should we move the imports to after any initial comment in the file?


I don't think it'll matter. Top of the file should be fine.

mary-poppins · 2018-07-17T13:36:53Z

You can preview 493098b at https://pr24897-493098b.ngbuilds.io/.

mary-poppins · 2018-07-18T12:56:52Z

You can preview 51cc660 at https://pr24897-51cc660.ngbuilds.io/.

mary-poppins · 2018-07-18T13:55:38Z

You can preview 082c994 at https://pr24897-082c994.ngbuilds.io/.

googlebot · 2018-07-23T06:19:31Z

CLAs look good, thanks!

This supports use cases needed by ngcc, where the compilation needs to be configured for JavaScript differently to normal TypeScript. PR Close #24897

PR Close #24897

gkalpak · 2018-07-28T09:55:08Z

AFAICT, this change was dropped from this PR (possibly due to a bad rebase?).
I will add it back in #25090.

angular-automatic-lock-bot · 2019-09-13T21:21:58Z

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.}

petebacondarwin added this to the Ivy milestone Jul 16, 2018

petebacondarwin requested review from alxhub, gkalpak and mhevery July 16, 2018 09:31

googlebot added the cla: no label Jul 16, 2018

petebacondarwin commented Jul 16, 2018

View reviewed changes

petebacondarwin force-pushed the ngcc branch 3 times, most recently from ae04929 to ee37256 Compare July 16, 2018 13:19

alxhub requested changes Jul 17, 2018

View reviewed changes

petebacondarwin force-pushed the ngcc branch from ee37256 to 493098b Compare July 17, 2018 13:26

petebacondarwin force-pushed the ngcc branch 2 times, most recently from da93817 to 082c994 Compare July 18, 2018 13:44

petebacondarwin force-pushed the ngcc branch from 082c994 to 8a7b0e9 Compare July 23, 2018 06:19

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

test(ivy): allow makeProgram to be more configurable (#24897)

07e6de5

This supports use cases needed by ngcc, where the compilation needs to be configured for JavaScript differently to normal TypeScript. PR Close #24897

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

build: add dependencies to be used by ngcc (#24897)

a673494

PR Close #24897

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

feat(ivy): ngcc project skeleton (#24897)

d7aa20d

PR Close #24897

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

test(ivy): implement ngcc specific version of makeProgram (#24897)

4ad2f11

PR Close #24897

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

feat(ivy): implement esm2015 and esm5 reflection hosts (#24897)

45cf5b5

PR Close #24897

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

feat(ivy): implement esm2015 and esm5 file parsers (#24897)

2f70e90

PR Close #24897

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

feat(ivy): implement ngcc Analyzer (#24897)

844d510

PR Close #24897

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

feat(ivy): implement esm2015 and esm5 ngcc file renderers (#24897)

5b32aa4

PR Close #24897

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

feat(ivy): implement initial ngcc package transformer (#24897)

43d0e3d

PR Close #24897

IgorMinar pushed a commit that referenced this pull request Jul 28, 2018

refactor(ivy): do not deep import from ngtsc into ngcc (#24897)

a87d44c

PR Close #24897

gkalpak mentioned this pull request Jul 28, 2018

feat(ivy): fix and enable esm2015/esm5 processing in ngcc #25090

Closed

3 tasks

gkalpak deleted the ngcc branch July 28, 2018 10:43

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019

Conversation

petebacondarwin commented Jul 16, 2018

PR Checklist

PR Type

Does this PR introduce a breaking change?

Uh oh!

googlebot commented Jul 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jul 16, 2018

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jul 17, 2018

Uh oh!

mary-poppins commented Jul 18, 2018

Uh oh!

mary-poppins commented Jul 18, 2018

Uh oh!

googlebot commented Jul 23, 2018

Uh oh!

gkalpak commented Jul 28, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

petebacondarwin Jul 18, 2018 •

edited

Loading