fix(ngcc): do not collect private declarations from external packages #34811

gkalpak · 2020-01-16T11:38:44Z

Previously, while trying to build an NgccReflectionHost's privateDtsDeclarationMap, computePrivateDtsDeclarationMap() would try to collect exported declarations from all source files of the program (i.e. without checking whether they were within the target entry-point, as happens for declarations in .d.ts files).

Most of the time, that would not be a problem, because external packages would be represented as .d.ts files in the program. But when an external package had no typings, the JS files would be used instead. As a result, the ReflectionHost would try to (unnecessarilly) parse the file in order to extract exported declarations, which in turn would be harmless in most cases.

There are certain cases, though, where the ReflectionHost would throw an error, because it cannot not parse the external package's JS file. This could happen, for example, in UmdReflectionHost, which expects the file to contain exactly one statement. See #34544 for more details on a real-world failure.

This commit fixes the issue by ensuring that computePrivateDtsDeclarationMap() will only collect exported declarations from files within the target entry-point.

Jira issue: FW-1794

Fixes #34544.

For good measure, I have also tested it on the ngcc-validation repo (angular/ngcc-validation#777) and nothing broke.

mary-poppins · 2020-01-16T12:14:24Z

You can preview a8b07f6 at https://pr34811-a8b07f6.ngbuilds.io/.

mary-poppins · 2020-01-16T19:03:39Z

You can preview 38e9d9e at https://pr34811-38e9d9e.ngbuilds.io/.

petebacondarwin

A couple of nits/questions.

Please change references to "within the entry-point" to "within the package" in the commit message. We do not (and cannot reliably) check whether source files are within a specific entry-point.

petebacondarwin · 2020-01-17T07:56:15Z

integration/run_tests.sh

@@ -38,7 +38,7 @@ fi

 # Workaround https://github.com/yarnpkg/yarn/issues/2165
 # Yarn will cache file://dist URIs and not update Angular code
-readonly cache=.yarn_local_cache
+export readonly cache=.yarn_local_cache


OOC what difference does this make?

Exporting it? It makes it available to other scripts called by this script (i.e. the ngcc/test.sh script).

packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts

petebacondarwin · 2020-01-17T08:15:59Z

packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts

+
+        loadTestFiles(TYPINGS_SRC_FILES);
+        loadTestFiles(TYPINGS_DTS_FILES);
+        const externalLibWithoutTypingsIndex = _('/an_external_lib_without_typings/index.js');


NIT: could we move this declaration to above the TestEsm2015ReflectionHost class declaration since it is used in that class.

petebacondarwin · 2020-01-17T08:17:35Z

packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts

+        class TestEsm2015ReflectionHost extends Esm2015ReflectionHost {
+          getExportsOfModule(node: ts.Node) {
+            if (ts.isSourceFile(node) && (node.fileName === externalLibWithoutTypingsIndex)) {
+              throw new Error('Test error');


Can you make this error more descriptive. Getting a failure in a test that just says "Test error" is not so helpful.

petebacondarwin · 2020-01-17T08:23:30Z

packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts

+        loadTestFiles(TYPINGS_DTS_FILES);
+        const externalLibWithoutTypingsIndex = _('/an_external_lib_without_typings/index.js');
+        const bundle = makeTestBundleProgram(
+            getRootFiles(TYPINGS_SRC_FILES)[0], false, [externalLibWithoutTypingsIndex]);


Do you actually need to add this as an additional file? Since it is imported from the entry-point, I would expect it to be included in the program's source files automatically.

That's what I expected too. But it is not included 😕

…sOfModule()` error message

Previously, while trying to build an `NgccReflectionHost`'s `privateDtsDeclarationMap`, `computePrivateDtsDeclarationMap()` would try to collect exported declarations from all source files of the program (i.e. without checking whether they were within the target package, as happens for declarations in `.d.ts` files). Most of the time, that would not be a problem, because external packages would be represented as `.d.ts` files in the program. But when an external package had no typings, the JS files would be used instead. As a result, the `ReflectionHost` would try to (unnecessarilly) parse the file in order to extract exported declarations, which in turn would be harmless in most cases. There are certain cases, though, where the `ReflectionHost` would throw an error, because it cannot parse the external package's JS file. This could happen, for example, in `UmdReflectionHost`, which expects the file to contain exactly one statement. See angular#34544 for more details on a real-world failure. This commit fixes the issue by ensuring that `computePrivateDtsDeclarationMap()` will only collect exported declarations from files within the target package. Jira issue: [FW-1794](https://angular-team.atlassian.net/browse/FW-1794) Fixes angular#34544

…ackages

mary-poppins · 2020-01-18T19:23:35Z

You can preview 74e8273 at https://pr34811-74e8273.ngbuilds.io/.

kainiedziela · 2020-01-22T22:11:15Z

Would love to see this included in 9.0.0-rc.11

josephperrott

Approval for integration

AndrewKushnir · 2020-01-23T21:55:52Z

JFYI, the only change that affected g3 is the error message text (function name correction) in packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts, so no g3 presubmit is required.

…sOfModule()` error message (#34811) PR Close #34811

…#34811) Previously, while trying to build an `NgccReflectionHost`'s `privateDtsDeclarationMap`, `computePrivateDtsDeclarationMap()` would try to collect exported declarations from all source files of the program (i.e. without checking whether they were within the target package, as happens for declarations in `.d.ts` files). Most of the time, that would not be a problem, because external packages would be represented as `.d.ts` files in the program. But when an external package had no typings, the JS files would be used instead. As a result, the `ReflectionHost` would try to (unnecessarilly) parse the file in order to extract exported declarations, which in turn would be harmless in most cases. There are certain cases, though, where the `ReflectionHost` would throw an error, because it cannot parse the external package's JS file. This could happen, for example, in `UmdReflectionHost`, which expects the file to contain exactly one statement. See #34544 for more details on a real-world failure. This commit fixes the issue by ensuring that `computePrivateDtsDeclarationMap()` will only collect exported declarations from files within the target package. Jira issue: [FW-1794](https://angular-team.atlassian.net/browse/FW-1794) Fixes #34544 PR Close #34811

…sOfModule()` error message (angular#34811) PR Close angular#34811

…angular#34811) Previously, while trying to build an `NgccReflectionHost`'s `privateDtsDeclarationMap`, `computePrivateDtsDeclarationMap()` would try to collect exported declarations from all source files of the program (i.e. without checking whether they were within the target package, as happens for declarations in `.d.ts` files). Most of the time, that would not be a problem, because external packages would be represented as `.d.ts` files in the program. But when an external package had no typings, the JS files would be used instead. As a result, the `ReflectionHost` would try to (unnecessarilly) parse the file in order to extract exported declarations, which in turn would be harmless in most cases. There are certain cases, though, where the `ReflectionHost` would throw an error, because it cannot parse the external package's JS file. This could happen, for example, in `UmdReflectionHost`, which expects the file to contain exactly one statement. See angular#34544 for more details on a real-world failure. This commit fixes the issue by ensuring that `computePrivateDtsDeclarationMap()` will only collect exported declarations from files within the target package. Jira issue: [FW-1794](https://angular-team.atlassian.net/browse/FW-1794) Fixes angular#34544 PR Close angular#34811

angular-automatic-lock-bot · 2020-02-23T16:09:10Z

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 type: bug/fix state: WIP target: patch This PR is targeted for the next patch release comp: ngcc labels Jan 16, 2020

ngbot bot modified the milestone: needsTriage Jan 16, 2020

googlebot added the cla: yes label Jan 16, 2020

gkalpak force-pushed the fix-ngcc-ignore-nodejs-builtins branch from 53c2d7b to a8b07f6 Compare January 16, 2020 12:05

gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Jan 16, 2020

Test PR angular/angular#34811

6ecdba9

gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Jan 16, 2020

Test PR angular/angular#34811

6389c75

gkalpak force-pushed the fix-ngcc-ignore-nodejs-builtins branch 2 times, most recently from eb0c337 to 38e9d9e Compare January 16, 2020 18:54

gkalpak added action: merge The PR is ready for merge by the caretaker and removed state: WIP labels Jan 16, 2020

gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Jan 16, 2020

Test PR angular/angular#34811

b2a282c

gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Jan 16, 2020

Test PR angular/angular#34811

d095c53

gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Jan 16, 2020

Test PR angular/angular#34811

54ad524

gkalpak marked this pull request as ready for review January 16, 2020 20:39

gkalpak requested review from a team as code owners January 16, 2020 20:39

gkalpak mentioned this pull request Jan 16, 2020

platform-server errors with ivy-ngcc - UMD module - Angular 9.0.0-rc.7 #34544

Closed

kapunahelewong approved these changes Jan 16, 2020

View reviewed changes

petebacondarwin approved these changes Jan 17, 2020

View reviewed changes

petebacondarwin removed the action: merge The PR is ready for merge by the caretaker label Jan 17, 2020

petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 17, 2020

gkalpak added 3 commits January 18, 2020 20:28

refactor(compiler-cli): fix typo in `TypeScriptCompilerHost#getExport…

2fe8897

…sOfModule()` error message

fixup! fix(ngcc): do not collect private declarations from external p…

74e8273

…ackages

gkalpak force-pushed the fix-ngcc-ignore-nodejs-builtins branch from 38e9d9e to 74e8273 Compare January 18, 2020 19:15

gkalpak added action: merge The PR is ready for merge by the caretaker 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 18, 2020

alxhub self-assigned this Jan 23, 2020

alxhub approved these changes Jan 23, 2020

View reviewed changes

josephperrott approved these changes Jan 23, 2020

View reviewed changes

AndrewKushnir pushed a commit that referenced this pull request Jan 23, 2020

refactor(compiler-cli): fix typo in `TypeScriptCompilerHost#getExport…

92d26e2

…sOfModule()` error message (#34811) PR Close #34811

AndrewKushnir closed this in ba2bf82 Jan 23, 2020

gkalpak deleted the fix-ngcc-ignore-nodejs-builtins branch January 23, 2020 22:04

sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020

refactor(compiler-cli): fix typo in `TypeScriptCompilerHost#getExport…

30604b3

…sOfModule()` error message (angular#34811) PR Close angular#34811

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngcc): do not collect private declarations from external packages #34811

fix(ngcc): do not collect private declarations from external packages #34811

gkalpak commented Jan 16, 2020 •

edited

mary-poppins commented Jan 16, 2020

mary-poppins commented Jan 16, 2020

petebacondarwin left a comment

petebacondarwin Jan 17, 2020

gkalpak Jan 18, 2020

petebacondarwin Jan 17, 2020

gkalpak Jan 18, 2020

petebacondarwin Jan 17, 2020

gkalpak Jan 18, 2020

petebacondarwin Jan 17, 2020

gkalpak Jan 18, 2020

mary-poppins commented Jan 18, 2020

kainiedziela commented Jan 22, 2020

josephperrott left a comment

AndrewKushnir commented Jan 23, 2020

angular-automatic-lock-bot bot commented Feb 23, 2020

fix(ngcc): do not collect private declarations from external packages #34811

fix(ngcc): do not collect private declarations from external packages #34811

Conversation

gkalpak commented Jan 16, 2020 • edited

mary-poppins commented Jan 16, 2020

mary-poppins commented Jan 16, 2020

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Jan 18, 2020

kainiedziela commented Jan 22, 2020

josephperrott left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Jan 23, 2020

angular-automatic-lock-bot bot commented Feb 23, 2020

gkalpak commented Jan 16, 2020 •

edited