feat(ivy): fix and enable esm2015/esm5 processing in ngcc by gkalpak · Pull Request #25090 · angular/angular

gkalpak · 2018-07-25T15:27:31Z

Sits on top of #25060.

PR Checklist

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

[x] Bugfix
[x] Feature
[x] Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

[ ] Yes
[x] No

googlebot · 2018-07-25T15:27:43Z

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.

mary-poppins · 2018-07-25T15:38:03Z

You can preview 1ce3cc5 at https://pr25090-1ce3cc5.ngbuilds.io/.

gkalpak · 2018-07-28T10:30:37Z

Rebased on #25060 (which was rebased on master, now that #24897 has been merged).
I also added back a commit per #24897 (comment).

mary-poppins · 2018-07-28T10:37:35Z

You can preview 9cdcb36 at https://pr25090-9cdcb36.ngbuilds.io/.

alxhub · 2018-08-07T18:01:06Z

+   * Check whether the given node actually represents a class.
   */
-  isClass(node: ts.Declaration): boolean;
+  isClass(node: ts.Node): boolean;


Can you describe the rationale for this change? In my mind the class should always be a declaration.

In ES6:

class Foo {} // ts.ClassDeclaration

In ES5:

// ts.VariableDeclaration var Foo = /** @class */ (function () { function Foo() { } return Foo; }());

The rational is described in Esm5ReflectionHost's getClassSymbol() method: https://github.com/angular/angular/pull/25090/files#diff-ac5866175696e5b67c53651096f39094R49

Basically, in some cases we end up passing the inner function declaration (from inside the IIFE) to isClass().

Maybe there is a better way to avoid that, but I couldn't find any.

alxhub · 2018-08-07T18:08:48Z

+      // It might be the function expression inside the IIFE. We need to go 5 levels up...
+
+      // 1. IIFE body.
+      let outerNode = node.parent;


Just a cautionary note - different versions of TypeScript generate slightly different syntax for class declarations ;)

😞
No idea how to work around it other than having a different implementation for each version of TS 😒

alxhub · 2018-08-07T18:17:52Z

+
+      return this.checker.getSymbolAtLocation(innerClassIdentifier);
+    }
+


else if for clarity?

alxhub · 2018-08-07T18:18:38Z

-      const options: ts.CompilerOptions = {allowJs: true, rootDir: entryPointPath};
+      const options: ts.CompilerOptions = {
+        allowJs: true,
+        maxNodeModuleJsDepth: Infinity,


Nice catch on this one.

alxhub · 2018-08-07T18:21:05Z

+      const barNode =
+          getDeclaration(program, FUNCTION_BODY_FILE.name, 'bar', ts.isFunctionDeclaration) !;
+      const barDef = host.getDefinitionOfFunction(barNode);
+      expect(barDef.node).toBe(barNode);


Geez, we need a better way to write tests like these. I'm gonna work on that.

alxhub · 2018-08-07T18:21:54Z

+    const commonPath = path.join(nodeModulesPath, '@angular/common');
+    const exitCode = mainNgcc([commonPath, 'esm2015']);
+
+    console.warn(find('node_modules_ngtsc').filter(p => p.endsWith('.js') || p.endsWith('map')));


Are these console.warn statements intentional?

Yes 😁
At this point, the e2e tests consist of us looking at the printed result and concluding it "looks reasonable". We will have better e2e tests soon 😁

(Note, the console.warns are not introduced in this PR btw; they are already present in other tests.)

petebacondarwin · 2018-08-08T09:07:34Z

+      } else if (entry instanceof Reference) {
+        if (!entry.expressable) {
+          throw new Error(`Value at position ${idx} in ${name} array is not expressable`);
+        } else if (!this.reflector.isClass(entry.node)) {


if we add an additional ts.isDeclaration(entry.node) here then we don't need to change the signature of isClass.

But this will potentially hide errors (where entry.node is a Declaration but not a class) and not help in other places where isClass() might be called with the inner function declaration on ESM5.

Generally, I think this should be the reflector's concern.

I looked again and I agree that we need to change the isClass to take a ts.Node.

ngtsc's static resolver can evaluate function calls where parameters have default values. In TypeScript code these default values live on the function definition, but in ES5 code the default values are represented by statements in the function body. A new ReflectionHost method getDefinitionOfFunction() abstracts over this difference, and allows the static reflector to more accurately evaluate ES5 code.

…ection hosts

In some code formats (e.g. ES5) methods can actually be function expressions. For example: ```js function MyClass() {} // this static method is declared as a function expression MyClass.staticMethod = function() { ... }; ```

Since non-flat module formats (esm2015, esm5) have different structure than their flat counterparts (and since we are operating on JS files inside `node_modules/`, we need to configure TS to include deeply nested JS files (by specifying a sufficiently high `maxNodeModuleJsDepth`). Remains to be determined if this has any (noticeable) performance implications.

gkalpak · 2018-08-20T13:49:01Z

Rebased on master and addressed (some) comments.

mary-poppins · 2018-08-20T13:54:24Z

You can preview af5a439 at https://pr25090-af5a439.ngbuilds.io/.

petebacondarwin · 2018-08-20T18:31:17Z

@gkalpak we have rolled this PR into the ngcc-integration-test branch and associated PR: #25406

We need to add any changes you make here to that, because @alxhub is going to try to land the whole lot in one go.

petebacondarwin · 2018-08-21T22:02:31Z

Superceded by #25203

angular-automatic-lock-bot · 2019-09-13T23:51:54Z

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

googlebot added the cla: no label Jul 25, 2018

gkalpak added the comp: ivy label Jul 25, 2018

gkalpak requested a review from petebacondarwin July 25, 2018 15:28

gkalpak added type: bug/fix feature Label used to distinguish feature request from other issues action: review The PR is still awaiting reviews from at least one requested reviewer refactoring Issue that involves refactoring or code-cleanup labels Jul 25, 2018

gkalpak mentioned this pull request Jul 28, 2018

feat(ivy): initial ngcc code dump #24897

Closed

3 tasks

gkalpak force-pushed the feat-ngcc-non-flat-formats branch from 1ce3cc5 to 9cdcb36 Compare July 28, 2018 10:28

gkalpak mentioned this pull request Jul 30, 2018

feat(ivy): add support for typings in ngcc (and some minor refactorings) #25203

Closed

3 tasks

gkalpak requested a review from alxhub July 30, 2018 20:23

alxhub approved these changes Aug 7, 2018

View reviewed changes

petebacondarwin reviewed Aug 8, 2018

View reviewed changes

alxhub and others added 8 commits August 20, 2018 16:35

feat(ivy): implement getDefinitionOfFunction on ES2015 and ES5 refl…

f479fd5

…ection hosts

refactor(ivy): remove unused arg from ngcc Analyzer

d3fe384

fix(ivy): correctly detect classes in ngcc Esm5ReflectionHost

613bc8b

feat(ivy): enable processing of esm5 format in ngcc

b60e4b9

fixup! fix(ivy): correctly detect classes in ngcc Esm5ReflectionHost

af5a439

gkalpak force-pushed the feat-ngcc-non-flat-formats branch from 9cdcb36 to af5a439 Compare August 20, 2018 13:47

petebacondarwin mentioned this pull request Aug 21, 2018

Ngcc es5 resolving #25060

Closed

petebacondarwin closed this Aug 21, 2018

gkalpak deleted the feat-ngcc-non-flat-formats branch August 22, 2018 16:01

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


		return this.checker.getSymbolAtLocation(innerClassIdentifier);
		}

Conversation

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

Uh oh!

PR Checklist

PR Type

Does this PR introduce a breaking change?

Uh oh!

googlebot commented Jul 25, 2018

Uh oh!

mary-poppins commented Jul 25, 2018

Uh oh!

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

Uh oh!

Uh oh!

mary-poppins commented Jul 28, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Aug 20, 2018

Uh oh!

mary-poppins commented Aug 20, 2018

Uh oh!

petebacondarwin commented Aug 20, 2018

Uh oh!

petebacondarwin commented Aug 21, 2018

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gkalpak commented Jul 25, 2018 •

edited

Loading

gkalpak commented Jul 28, 2018 •

edited

Loading