fix(ivy): several small fixes to run cli-hello-world-ivy integration test suite #27438

JoostK · 2018-12-03T21:45:21Z

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

What is the current behavior?

The cli-hello-world-ivy test would only build, not actually run. With some Ivy fixes the tests can now actually be enabled:

Fixes:

Pipes would not be available to JIT compiled modules.
Several __POST_R3__ symbols were DCE'd. The integration test now finally catches those.
The setClassMetadata would fail to be generated from ngcc for ES5 bundles.

This builds on top of #27159 as the ESM5 bundle is used for tests.

What is the new behavior?

Ivy Hello World integration test actually runs 🎉

Does this PR introduce a breaking change?

Yes
No

Other information

JoostK · 2018-12-03T21:56:53Z

packages/core/src/render3/jit/module.ts

@@ -51,6 +51,10 @@ export function compileNgModuleDefs(moduleType: Type<any>, ngModule: NgModule):
              exports: flatten(ngModule.exports || EMPTY_ARRAY).map(expandModuleWithProviders),
              emitInline: true,
            });
+
+        if (applyScope) {


This is a bit of a hack, but TestBed applies its own module scopes which this would otherwise interfere with. I have some work in #27399 to hopefully avoid this in the future, but that needs some more work.

JoostK · 2018-12-03T21:59:18Z

packages/core/src/render3/jit/module.ts

@@ -198,7 +202,7 @@ export function transitiveScopesFor<T>(moduleType: Type<T>): NgModuleTransitiveS
        scopes.compilation.pipes.add(entry);
        scopes.exported.pipes.add(entry);
      });
-    } else if (getNgModuleDef(exportedTyped)) {
+    } else if (getPipeDef(exportedTyped)) {


This bug was not found with TestBed tests as it has its own idea of module scopes, so this code was not relevant there. With the work in #27399 I deleted the custom module scope application (for now, we might still need something similar as my current approach seems too limited) and that uncovered this issue.

JoostK · 2018-12-03T22:39:10Z

I'll have to investigate why Travis fails with Error: Importing BrowserDynamicTestingModule which does not have an @ngModule. Somehow the setClassMetaData call is not present in the FESM5 bundles. I have seen this working locally but now also have the same error, not sure what has changed 🤷‍♂️

gkalpak

I have no idea about the setScopeOnDeclaredComponents() change.
The rest looks great 😃

pkozlowski-opensource · 2018-12-04T15:21:19Z

@JoostK @gkalpak I think that TestBed is definitively doing sth fishy with scopes, I remember discussing it briefly with @alxhub in the past.

JoostK · 2018-12-04T16:14:54Z

@JoostK @gkalpak I think that TestBed is definitively doing sth fishy with scopes, I remember discussing it briefly with @alxhub in the past.

Yes, it definitely is. Basically TestBed has its own module scoping as it walks the module graph to recompile everything anyway based on the registered overrides. That's what #27399 is supposed to avoid, although the module scope modifications prove to be problematic.

JoostK · 2018-12-04T22:31:58Z

@gkalpak All green now 🍏 💚 I've had to disable the ci-production e2e configuration as that would timeout with an error that Angular cannot be found. Don't know what's going on exactly but I'm accepting for now that that test just doesn't run yet.

gkalpak · 2018-12-05T13:01:42Z

packages/compiler-cli/src/ngcc/src/main.ts

+        if (checkMarkerFile(entryPoint, format)) {
+          console.warn(`Skipping ${entryPoint.name} : ${format} (already built).`);
+          return;
+        }


gkalpak · 2018-12-05T13:06:53Z

integration/cli-hello-world-ivy/package.json

    "start": "ng serve",
-    "test": "ng build --progress false"
+    "test": "ng test --progress=false --watch=false && yarn e2e --configuration=ci"


Can you leave a TODO comment here mentioning that ci-production is failing? E.g.:

"//test1": "TODO: Re-enable `ci-production`. Currently, it fails like this and that.", "//test2": "ng test --progress=false --watch=false && yarn e2e --configuration=ci && yarn e2e --configuration=ci-production",

@gkalpak Good idea, done!

Thx! Can you also add that this is tracked in Jira issue is FW-813?

@gkalpak Done, thanks for registering in Jira 👍

gkalpak · 2018-12-07T11:16:31Z

integration/cli-hello-world-ivy/package.json

    "start": "ng serve",
-    "test": "ng build --progress false"
+    "test": "ng test --progress=false --watch=false && yarn e2e --configuration=ci"


Thx! Can you also add that this is tracked in Jira issue is FW-813?

gkalpak · 2018-12-12T13:48:33Z

Marking this as blocked on #27159. (I'd prefer to merge that first in order to preserve the PR comment history.)

alxhub · 2018-12-13T01:41:10Z

Nice work!

alxhub

Waiting to rebase once #27159 lands

While creating FESM files, rollup usually drops all unused symbols. All *__POST_R3__ are unused unless ngcc rewires stuff. To prevent this DCE we reexport them as private symbols. If ngcc is not used, these symbols will be dropped when we optimize an application bundle.

This is tested in the Ivy CLI integration test.

ngcc would feed ngtsc with the function declaration inside of an IIFE as that is considered the class symbol's declaration node, according to TypeScript's `ts.Symbol.valueDeclaration`. ngtsc however only considered variable decls and actual class decls as potential class declarations, so given the function declaration node it would fail to generate the `setClassMetadata` call. ngtsc no longer makes its own assumptions about what classes look like, but always asks the reflection host to yield this kind of information.

With the bundle info being assembled into a single object before the transform is started, we now greedily create a TypeScript program up-front. If a marker file exists that indicates that the bundle could be skipped the program creation has already taken place which takes a significant amount of time. This commit moves the marker check to occur before the bundle is assembled.

…ration suite The ci-production e2e test is diabled because Angular fails to be found

JoostK · 2018-12-15T19:59:54Z

Rebased now that #27159 has been merged. Tests are green, presubmit time!

gkalpak · 2018-12-16T20:38:55Z

Note to caretaker: Assistance needed with g3sync.

This is tested in the Ivy CLI integration test. PR Close #27438

ngcc would feed ngtsc with the function declaration inside of an IIFE as that is considered the class symbol's declaration node, according to TypeScript's `ts.Symbol.valueDeclaration`. ngtsc however only considered variable decls and actual class decls as potential class declarations, so given the function declaration node it would fail to generate the `setClassMetadata` call. ngtsc no longer makes its own assumptions about what classes look like, but always asks the reflection host to yield this kind of information. PR Close #27438

…27438) With the bundle info being assembled into a single object before the transform is started, we now greedily create a TypeScript program up-front. If a marker file exists that indicates that the bundle could be skipped the program creation has already taken place which takes a significant amount of time. This commit moves the marker check to occur before the bundle is assembled. PR Close #27438

…ration suite (#27438) The ci-production e2e test is diabled because Angular fails to be found PR Close #27438

…27438) PR Close #27438

While creating FESM files, rollup usually drops all unused symbols. All *__POST_R3__ are unused unless ngcc rewires stuff. To prevent this DCE we reexport them as private symbols. If ngcc is not used, these symbols will be dropped when we optimize an application bundle. PR Close angular#27438

This is tested in the Ivy CLI integration test. PR Close angular#27438

…lar#27438) ngcc would feed ngtsc with the function declaration inside of an IIFE as that is considered the class symbol's declaration node, according to TypeScript's `ts.Symbol.valueDeclaration`. ngtsc however only considered variable decls and actual class decls as potential class declarations, so given the function declaration node it would fail to generate the `setClassMetadata` call. ngtsc no longer makes its own assumptions about what classes look like, but always asks the reflection host to yield this kind of information. PR Close angular#27438

…ngular#27438) With the bundle info being assembled into a single object before the transform is started, we now greedily create a TypeScript program up-front. If a marker file exists that indicates that the bundle could be skipped the program creation has already taken place which takes a significant amount of time. This commit moves the marker check to occur before the bundle is assembled. PR Close angular#27438

…ration suite (angular#27438) The ci-production e2e test is diabled because Angular fails to be found PR Close angular#27438

…ngular#27438) PR Close angular#27438

angular-automatic-lock-bot · 2019-09-14T10:53:01Z

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: yes label Dec 3, 2018

JoostK commented Dec 3, 2018

View reviewed changes

gkalpak reviewed Dec 4, 2018

View reviewed changes

JoostK force-pushed the ivy-green-integration-test branch 3 times, most recently from 6da5be0 to fcf4948 Compare December 4, 2018 22:01

gkalpak reviewed Dec 5, 2018

View reviewed changes

JoostK force-pushed the ivy-green-integration-test branch 2 times, most recently from ae877e4 to b48bcf8 Compare December 6, 2018 22:25

gkalpak approved these changes Dec 7, 2018

View reviewed changes

JoostK force-pushed the ivy-green-integration-test branch 4 times, most recently from 316d64e to 3902d92 Compare December 10, 2018 18:45

gkalpak added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Dec 12, 2018

ngbot bot added this to the needsTriage milestone Dec 12, 2018

gkalpak added the state: blocked label Dec 12, 2018

alxhub approved these changes Dec 13, 2018

View reviewed changes

JoostK added 3 commits December 15, 2018 17:25

fix(ivy): make exported pipes available in module scope

2384a96

This is tested in the Ivy CLI integration test.

JoostK added 2 commits December 15, 2018 17:25

test(ivy): synchronize cli-hello-world-ivy with cli-hello-world integ…

1a0bb1c

…ration suite The ci-production e2e test is diabled because Angular fails to be found

JoostK force-pushed the ivy-green-integration-test branch 2 times, most recently from ade4563 to a455354 Compare December 15, 2018 18:28

test(ivy): enable running cli-hello-world-ivy integration test suite

28f4dd7

JoostK force-pushed the ivy-green-integration-test branch from a455354 to 28f4dd7 Compare December 15, 2018 18:41

gkalpak removed the state: blocked label Dec 16, 2018

gkalpak approved these changes Dec 16, 2018

View reviewed changes

mhevery closed this in 1160025 Dec 17, 2018

mhevery pushed a commit that referenced this pull request Dec 17, 2018

fix(ivy): make exported pipes available in module scope (#27438)

84084b1

This is tested in the Ivy CLI integration test. PR Close #27438

mhevery pushed a commit that referenced this pull request Dec 17, 2018

test(ivy): synchronize cli-hello-world-ivy with cli-hello-world integ…

b5b229d

…ration suite (#27438) The ci-production e2e test is diabled because Angular fails to be found PR Close #27438

mhevery pushed a commit that referenced this pull request Dec 17, 2018

test(ivy): enable running cli-hello-world-ivy integration test suite (#…

44ec66d

…27438) PR Close #27438

ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

fix(ivy): make exported pipes available in module scope (angular#27438)

618cf12

This is tested in the Ivy CLI integration test. PR Close angular#27438

ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

test(ivy): enable running cli-hello-world-ivy integration test suite (a…

1ca6752

…ngular#27438) PR Close angular#27438

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): several small fixes to run cli-hello-world-ivy integration test suite #27438

fix(ivy): several small fixes to run cli-hello-world-ivy integration test suite #27438

JoostK commented Dec 3, 2018 •

edited

JoostK Dec 3, 2018

JoostK Dec 3, 2018

JoostK commented Dec 3, 2018

gkalpak left a comment

pkozlowski-opensource commented Dec 4, 2018

JoostK commented Dec 4, 2018

JoostK commented Dec 4, 2018

gkalpak Dec 5, 2018

gkalpak Dec 5, 2018

JoostK Dec 6, 2018

gkalpak Dec 7, 2018

JoostK Dec 7, 2018

gkalpak Dec 7, 2018

gkalpak commented Dec 12, 2018

alxhub commented Dec 13, 2018

alxhub left a comment •

edited

JoostK commented Dec 15, 2018

gkalpak commented Dec 16, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

fix(ivy): several small fixes to run cli-hello-world-ivy integration test suite #27438

fix(ivy): several small fixes to run cli-hello-world-ivy integration test suite #27438

Conversation

JoostK commented Dec 3, 2018 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK commented Dec 3, 2018

gkalpak left a comment

Choose a reason for hiding this comment

pkozlowski-opensource commented Dec 4, 2018

JoostK commented Dec 4, 2018

JoostK commented Dec 4, 2018

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

gkalpak commented Dec 12, 2018

alxhub commented Dec 13, 2018

alxhub left a comment • edited

Choose a reason for hiding this comment

JoostK commented Dec 15, 2018

gkalpak commented Dec 16, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

JoostK commented Dec 3, 2018 •

edited

alxhub left a comment •

edited