fix(ngcc): several fixes #34169

gkalpak · 2019-12-01T17:43:54Z

This PR includes several ngcc fixes. With these fixes, ngcc should be able to successfully process NativeScript. See individual commits for details.

Jira issue: FW-1689

TODO:

Determine whether it is problematic that the synthetically created function declaration for TS helpers will be recognized as non-synthetic in StaticInterpreter (and how to fix it).
Implement the CommonJS fixes for UMD as well.
Add integration tests.

mary-poppins · 2019-12-01T17:50:16Z

You can preview 5fdc4a2 at https://pr34169-5fdc4a2.ngbuilds.io/.

mary-poppins · 2019-12-01T18:26:54Z

You can preview 263efda at https://pr34169-263efda.ngbuilds.io/.

mary-poppins · 2019-12-02T19:28:33Z

You can preview 8077ce1 at https://pr34169-8077ce1.ngbuilds.io/.

mary-poppins · 2019-12-03T21:11:22Z

You can preview b0cc9d9 at https://pr34169-b0cc9d9.ngbuilds.io/.

mary-poppins · 2019-12-04T20:46:50Z

You can preview a45d1bb at https://pr34169-a45d1bb.ngbuilds.io/.

mary-poppins · 2019-12-05T16:47:38Z

You can preview 363218f at https://pr34169-363218f.ngbuilds.io/.

mary-poppins · 2019-12-05T18:47:28Z

You can preview 08f17b8 at https://pr34169-08f17b8.ngbuilds.io/.

mary-poppins · 2019-12-05T19:12:30Z

You can preview e28ac70 at https://pr34169-e28ac70.ngbuilds.io/.

petebacondarwin · 2019-12-05T21:03:04Z

typo in the 2nd to last commit: findDepeendencies

petebacondarwin · 2019-12-05T21:06:55Z

typos in the last commit:

In the case of emitted helpers, the tslib.__someHelper expression was
resolved to a function declaration of the form
export declare function __someHelper(...args: any[][]): any[];, which
allows getDefinitionOfFunction() to correctly map it to a TS helper.

I think this should be In the case of imported helpers, ....

And also functinos

petebacondarwin · 2019-12-06T08:59:17Z

typo in 5th commit: By failing to recongize

petebacondarwin

Reviewed all but the last comment.

petebacondarwin · 2019-12-06T08:52:43Z

packages/compiler-cli/ngcc/src/host/commonjs_host.ts

+  //       ([source](https://github.com/microsoft/TypeScript/blob/d7c83f023/src/compiler/transformers/module/module.ts#L1796-L1797)).
+  //       So, theoretically, we only care about the formats `__export(require('...'))` and
+  //       `tslib.__exportStar(require('...'), exports)`.
+  //       The accepts the other two formats (`__exportStar(...)` and `tslib.__export(...)`) to be


typo: This accepts

petebacondarwin · 2019-12-06T08:54:06Z

packages/compiler-cli/ngcc/src/host/commonjs_host.ts

+    fnName = statement.expression.expression.name.text;
+  }
+
+  // Esnure the called function is either `__export()` or `__exportStar()`.


typ: Ensure

petebacondarwin · 2019-12-06T09:04:28Z

packages/compiler-cli/ngcc/test/dependencies/commonjs_dependency_host_spec.ts

+  // tslib_1.__exportStar(require('...'), exports);
+  reExportsWithImportedHelper?: string[];
+}
+


NIT: I prefer not to clutter the start of a test file with supporting stuff (like helper functions and interfaces) since it makes it more difficult to quickly get to the tests when browsing later. Since interfaces are always hoisted, can we move this down below the describe block?

petebacondarwin · 2019-12-06T09:13:17Z

packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts

+                            ts.isPropertyAssignment(prop) && isRequireCall(prop.initializer))
+                    .map(prop => prop.initializer);
+
+            requireCalls.push(...requireCallsFromProperties);


NIT: this could be more simple and not require intermediate arrays to be created:

stmt.expression.right.properties.forEach(prop => { if(ts.isPropertyAssignment(prop) && isRequireCall(prop.initializer)) { requireCalls.push(prop.initializer); } });

packages/compiler-cli/ngcc/src/dependencies/commonjs_dependency_host.ts

petebacondarwin · 2019-12-06T10:54:55Z

packages/compiler-cli/ngcc/src/host/esm5_host.ts

+            ],
+            ts.createArrayTypeNode(ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword)), undefined),
+        viaModule: null,
+      };


NIT: I would put this whole block into its own function (e..g createSynthesizedHelperDeclaration(id)) with the big comment converted to JSDOC comment. This will keep the calling function much smaller and more readable.

petebacondarwin

The PR looks great! Regarding the last commit, I can accept this approach. I don't know what other ideas you had. One thought is if we could create a new thing called a LazyFunction, which we would match code of the form (this && this.<fn>) || function () { ... }, which could then be visited more intelligently in the StaticInterpreter?

petebacondarwin · 2019-12-11T18:04:09Z

@gkalpak is this PR nearly ready? Is it worth splitting out the non-contentious bits so that we can land them this week?

gkalpak · 2019-12-12T12:33:21Z

I'll split the non-contentious bits into a separate PR this week 👍

Previously, the `CommonJsReflectionHost` would only recognize re-exports of the form `__export(require('...'))`. This is what re-exports look like, when the TS helpers are emitted inline (i.e. when compiling with the default [TS compiler options][1] that include `noEmitHelpers: false` and `importHelpers: false`). However, when compiling with `importHelpers: true` and [tslib][2] (which is the recommended way for optimized bundles), the re-exports will look like: `tslib_1.__exportStar(require('...'), exports)` These types of re-exports where previously not recognized by `CommonJsReflectionHost` and thus ignored. This commit fixes this by ensuring both re-export formats are recognized. [1]: https://www.typescriptlang.org/docs/handbook/compiler-options.html [2]: https://www.npmjs.com/package/tslib

Previously, `CommonJsDependencyHost.findDependencies()` would only find dependencies via imports of the form `var foo = require('...');` or `var foo = require('...'), bar = require('...');` However, CommonJS files can have imports in many different forms. By failing to recognize other forms of imports, the associated dependencies were missed, which in turn resulted in entry-points being compiled out-of-order and failing due to that. While we cannot easily capture all different types of imports, this commit enhances `CommonJsDependencyHost` to recognize the following common forms of imports: - Imports in property assignments. E.g.: `exports.foo = require('...');` or `module.exports = {foo: require('...')};` - Imports for side-effects only. E.g.: `require('...');` - Star re-exports (with both emitted and imported heleprs). E.g.: `__export(require('...'));` or `tslib_1.__exportStar(require('...'), exports);`

In ES5 code, TypeScript requires certain helpers (such as `__spreadArrays()`) to be able to support ES2015+ features. These helpers can be either imported from `tslib` (by setting the `importHelpers` TS compiler option to `true`) or emitted inline (by setting the `importHelpers` and `noEmitHelpers` TS compiler options to `false`, which is also the default value for both). Ngtsc's `StaticInterpreter` (which is also used during ngcc processing) is able to statically evaluate some of these helpers (currently `__spread()` and `__spreadArrays()`), as long as `ReflectionHost#getDefinitionOfFunction()` correctly detects the declaration of the helper. For this to happen, the left-hand size of the corresponding call expression (i.e. `__spread(...)` or `tslib.__spread(...)`) must be evaluated as a function declaration for `getDefinitionOfFunction()` to be called with. In the case of imported helpers, the `tslib.__someHelper` expression was resolved to a function declaration of the form `export declare function __someHelper(...args: any[][]): any[];`, which allows `getDefinitionOfFunction()` to correctly map it to a TS helper. In contrast, in the case of emitted helpers (and regardless of the module format: `CommonJS`, `ESNext`, `UMD`, etc.)), the `__someHelper` identifier was resolved to a variable declaration of the form `var __someHelper = (this && this.__someHelper) || function () { ... }`, which upon further evaluation was categorized as a `DynamicValue` (prohibiting further evaluation by the `getDefinitionOfFunction()`). As a result of the above, TypeScript helpers were not evaluated in ES5 code. --- This commit fixes this by detecting such helpers in `Esm5ReflectionHost#getDeclarationOfIdentifier()` and (instead of the actual variable declaration) return a synthesized function declaration, identical to the one that would have been returned for the imported helper. This allows `getDefinitionOfFunction()` to correctly map it to a TS helper, allowing `StaticInterpreter` to statically evaluate the associated call expression.

gkalpak · 2019-12-17T10:08:33Z

@petebacondarwin: FYI, I have addressed your comments (in fixup commits - except for the commit message typos) and moved some of the commits into separate PRs: #34437, #34441

I need to further split this into more PRs (for easier reviewing) and add similar fixes for UMD (where appropriate).

mary-poppins · 2019-12-17T10:14:23Z

You can preview 8c37785 at https://pr34169-8c37785.ngbuilds.io/.

gkalpak · 2019-12-21T16:04:36Z

Further split this up into separate PRs per fix: #34512, #34527, #34528
_{(Each builds on top of the previous one, so you can either only review all commits on the last one or only review the new commits on each one.)}

gkalpak · 2020-01-14T11:54:13Z

With all the "spin-off" PRs merged, the only remaining fix for this PR is to correctly detect emitted TS helpers in ES5 (i.e. 0b89f17). (I need to clean it up or possibly rework the approach.)

This will also fix processing mobx-angular@3.1.0 (see angular/ngcc-validation#762).

gkalpak · 2020-02-06T17:37:29Z

The final fix is in ~~#35139~~ #35191. This PR is now obsolete. Closing...

angular-automatic-lock-bot · 2020-03-08T16:09:19Z

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

gkalpak added state: WIP comp: ngcc labels Dec 1, 2019

ngbot bot modified the milestone: needsTriage Dec 1, 2019

googlebot added the cla: yes label Dec 1, 2019

gkalpak force-pushed the fix-ngcc-nativescript branch from 263efda to 8077ce1 Compare December 2, 2019 19:19

gkalpak force-pushed the fix-ngcc-nativescript branch from 8077ce1 to b0cc9d9 Compare December 3, 2019 21:03

gkalpak force-pushed the fix-ngcc-nativescript branch from b0cc9d9 to a45d1bb Compare December 4, 2019 20:39

gkalpak force-pushed the fix-ngcc-nativescript branch from a45d1bb to 363218f Compare December 5, 2019 16:38

gkalpak force-pushed the fix-ngcc-nativescript branch from 363218f to 08f17b8 Compare December 5, 2019 18:38

gkalpak changed the title ~~WIP - some ngcc fixes~~ (ngcc): several ngcc fixes Dec 5, 2019

gkalpak changed the title ~~(ngcc): several ngcc fixes~~ fix(ngcc): several fixes Dec 5, 2019

gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: bug/fix and removed state: WIP labels Dec 5, 2019

gkalpak marked this pull request as ready for review December 5, 2019 18:58

gkalpak requested review from a team as code owners December 5, 2019 18:58

gkalpak force-pushed the fix-ngcc-nativescript branch from 08f17b8 to e28ac70 Compare December 5, 2019 19:03

petebacondarwin reviewed Dec 6, 2019

View reviewed changes

petebacondarwin approved these changes Dec 6, 2019

View reviewed changes

gkalpak mentioned this pull request Dec 10, 2019

Ivy: NGCC incorrectly compiles the "@ngxs/router-plugin" as UMD #34191

Closed

gkalpak force-pushed the fix-ngcc-nativescript branch from e28ac70 to 2b6e20e Compare December 17, 2019 10:03

gkalpak added 6 commits December 17, 2019 12:04

fixup! fix(ngcc): recognize more re-export formats in CommonJS

12e9773

fixup! fix(ngcc): correctly detect dependencies in CommonJS

1c42fb3

fixup! fix(ngcc): correctly detect emitted TS helpers in ES5

8c37785

gkalpak force-pushed the fix-ngcc-nativescript branch from 2b6e20e to 8c37785 Compare December 17, 2019 10:06

gkalpak added state: blocked and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 10, 2020

gkalpak mentioned this pull request Jan 14, 2020

fix(ngcc): libraries using spread in object literals cannot be processed #34661

Closed

gkalpak closed this Feb 6, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 8, 2020

gkalpak deleted the fix-ngcc-nativescript branch October 2, 2020 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngcc): several fixes #34169

fix(ngcc): several fixes #34169

gkalpak commented Dec 1, 2019 •

edited

Loading

mary-poppins commented Dec 1, 2019

mary-poppins commented Dec 1, 2019

mary-poppins commented Dec 2, 2019

mary-poppins commented Dec 3, 2019

mary-poppins commented Dec 4, 2019

mary-poppins commented Dec 5, 2019

mary-poppins commented Dec 5, 2019

mary-poppins commented Dec 5, 2019

petebacondarwin commented Dec 5, 2019

petebacondarwin commented Dec 5, 2019

petebacondarwin commented Dec 6, 2019

petebacondarwin left a comment

petebacondarwin Dec 6, 2019

petebacondarwin Dec 6, 2019

petebacondarwin Dec 6, 2019

petebacondarwin Dec 6, 2019

petebacondarwin Dec 6, 2019

petebacondarwin left a comment

petebacondarwin commented Dec 11, 2019

gkalpak commented Dec 12, 2019

gkalpak commented Dec 17, 2019 •

edited

Loading

mary-poppins commented Dec 17, 2019

gkalpak commented Dec 21, 2019

gkalpak commented Jan 14, 2020 •

edited

Loading

gkalpak commented Feb 6, 2020 •

edited

Loading

angular-automatic-lock-bot bot commented Mar 8, 2020

fix(ngcc): several fixes #34169

fix(ngcc): several fixes #34169

Conversation

gkalpak commented Dec 1, 2019 • edited Loading

mary-poppins commented Dec 1, 2019

mary-poppins commented Dec 1, 2019

mary-poppins commented Dec 2, 2019

mary-poppins commented Dec 3, 2019

mary-poppins commented Dec 4, 2019

mary-poppins commented Dec 5, 2019

mary-poppins commented Dec 5, 2019

mary-poppins commented Dec 5, 2019

petebacondarwin commented Dec 5, 2019

petebacondarwin commented Dec 5, 2019

petebacondarwin commented Dec 6, 2019

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Dec 6, 2019

Choose a reason for hiding this comment

petebacondarwin Dec 6, 2019

Choose a reason for hiding this comment

petebacondarwin Dec 6, 2019

Choose a reason for hiding this comment

petebacondarwin Dec 6, 2019

Choose a reason for hiding this comment

petebacondarwin Dec 6, 2019

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin commented Dec 11, 2019

gkalpak commented Dec 12, 2019

gkalpak commented Dec 17, 2019 • edited Loading

mary-poppins commented Dec 17, 2019

gkalpak commented Dec 21, 2019

gkalpak commented Jan 14, 2020 • edited Loading

gkalpak commented Feb 6, 2020 • edited Loading

angular-automatic-lock-bot bot commented Mar 8, 2020

gkalpak commented Dec 1, 2019 •

edited

Loading

gkalpak commented Dec 17, 2019 •

edited

Loading

gkalpak commented Jan 14, 2020 •

edited

Loading

gkalpak commented Feb 6, 2020 •

edited

Loading