fix(ivy): Forward refs now supported #27439

benlesh · 2018-12-03T21:59:02Z

Adds deferred execution of scope setting for modules such that forward refs can be supported in ivy. Design docs can be found at https://docs.google.com/document/d/11KTbybis9rt0cZgMKd1wo_IKb6y1PMU-RoTDVLTXK4Y/edit#

mary-poppins · 2018-12-03T22:03:28Z

You can preview c572156 at https://pr27439-c572156.ngbuilds.io/.

alxhub

This should probably be a feat(ivy) not fix(ivy) since forwardRefs were unsupported before.

Also, check if this fixes any more tests besides just this one.

alxhub · 2018-12-03T22:04:21Z

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

+  for (let i = moduleQueue.length - 1; i--;) {
+    const {moduleType, ngModule} = moduleQueue[i];
+
+    if (ngModule.declarations && ngModule.declarations.every(isResolvedDeclaration)) {


ngModule.declarations could be nested, e.g: [[[forwardRef(() => Cmp)]]].

alxhub · 2018-12-03T22:05:08Z

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

@@ -19,14 +20,46 @@ import {reflectDependencies} from './util';

 const EMPTY_ARRAY: Type<any>[] = [];

+const moduleQueue: Array<{moduleType: Type<any>, ngModule: NgModule}> = [];


Maybe make a named type for this?

alxhub · 2018-12-03T22:05:27Z

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

@@ -51,6 +51,9 @@ export function compileComponent(type: Type<any>, metadata: Component): void {
          error.push(`Did you run and wait for 'resolveComponentResources()'?`);
          throw new Error(error.join('\n'));
        }
+
+        flushModuleScopingQueueAsMuchAsPossible();


Add a comment here explaining what's happening.

I think this must be moved below the assignment of the compilation result of ngComponent below.

The flushModuleScopingQueueAsMuchAsPossible may find that the component can be patched. It sees that ngComponentDef is present and therefore reads it to patch its scope, which will re-enter here again. We should probably not do that, as it results in repeated compilation.

alxhub · 2018-12-03T22:05:47Z

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

+  }
+}
+
+/** Returns truthy if a declaration (fron an ngModule.declarations) is resolved. */


Coerce this to boolean.

alxhub · 2018-12-03T22:05:57Z

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

 /**
 * Compiles a module in JIT mode.
 *
 * This function automatically gets called when a class has a `@NgModule` decorator.
 */
 export function compileNgModule(moduleType: Type<any>, ngModule: NgModule = {}): void {
  compileNgModuleDefs(moduleType, ngModule);
-  setScopeOnDeclaredComponents(moduleType, ngModule);
+  enqueueModuleForDelayedScoping(moduleType, ngModule);


Add a comment here explaining what's happening.

benlesh · 2018-12-03T22:43:45Z

Requested changes made
Haven't addressed CI failures yet
Haven't changed commit message yet (mostly to not lose comment history), will do that after approval.

mary-poppins · 2018-12-03T22:47:07Z

You can preview 8bc38ba at https://pr27439-8bc38ba.ngbuilds.io/.

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

googlebot · 2018-12-04T17:52:44Z

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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-12-04T17:57:03Z

CLAs look good, thanks!

benlesh · 2018-12-04T17:58:02Z

Squashed and updated the commit message as requested... Will review CI failures once they're done running.

mary-poppins · 2018-12-04T18:02:07Z

You can preview 099157a at https://pr27439-099157a.ngbuilds.io/.

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

mary-poppins · 2018-12-04T18:10:13Z

You can preview 3aded9b at https://pr27439-3aded9b.ngbuilds.io/.

mary-poppins · 2018-12-04T20:13:30Z

You can preview 11a259d at https://pr27439-11a259d.ngbuilds.io/.

mary-poppins · 2018-12-04T20:19:31Z

You can preview d645daf at https://pr27439-d645daf.ngbuilds.io/.

JoostK · 2018-12-04T20:52:54Z

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

-      moduleQueue.slice(i, 1);
-      setScopeOnDeclaredComponents(moduleType, ngModule);
+  if (!flushingModuleQueue) {
+    flushingModuleQueue = true;


This dance could probably be avoided if we move flushModuleScopingQueueAsMuchAsPossible to after the completion of the ngComponentDef compilation (see #27439 (comment)). This hack is now required because ngComponentDef is not re-entrant.

I think you're right about the flush needing to be moved... However, I'm going to leave this defensive check in this flush, because flushing should be idempotent.

mary-poppins · 2018-12-04T21:00:05Z

You can preview cf143cc at https://pr27439-cf143cc.ngbuilds.io/.

mary-poppins · 2018-12-04T21:56:08Z

You can preview 3e51daf at https://pr27439-3e51daf.ngbuilds.io/.

mary-poppins · 2018-12-05T00:34:26Z

You can preview a2d42c1 at https://pr27439-a2d42c1.ngbuilds.io/.

ngbot · 2018-12-05T06:30:14Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "ci/circleci: test_aio_preview" is failing
     status "continuous-integration/travis-ci/pr" is failing
     forbidden labels detected: PR action: review
     status "code-review/pullapprove" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

Adds deferred execution of scope setting for modules such that forward refs can be supported in ivy. Design docs can be found at https://docs.google.com/document/d/11KTbybis9rt0cZgMKd1wo_IKb6y1PMU-RoTDVLTXK4Y/edit#

IgorMinar · 2018-12-05T17:53:43Z

I've rebased and force-pushed this PR

mary-poppins · 2018-12-05T17:57:39Z

You can preview a39e019 at https://pr27439-a39e019.ngbuilds.io/.

Adds deferred execution of scope setting for modules such that forward refs can be supported in ivy. Design docs can be found at https://docs.google.com/document/d/11KTbybis9rt0cZgMKd1wo_IKb6y1PMU-RoTDVLTXK4Y/edit# PR Close angular#27439

angular-automatic-lock-bot · 2019-09-14T09:52:32Z

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

benlesh added the comp: ivy label Dec 3, 2018

benlesh requested a review from alxhub December 3, 2018 21:59

googlebot added the cla: yes label Dec 3, 2018

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

alxhub reviewed Dec 3, 2018

View reviewed changes

alxhub approved these changes Dec 3, 2018

View reviewed changes

JoostK reviewed Dec 3, 2018

View reviewed changes

packages/core/src/render3/jit/module.ts Outdated Show resolved Hide resolved

googlebot added cla: no and removed cla: yes labels Dec 4, 2018

benlesh force-pushed the ivy-jit-forwardref-support branch from 3aded9b to 099157a Compare December 4, 2018 17:56

googlebot added cla: yes and removed cla: no labels Dec 4, 2018

JoostK reviewed Dec 4, 2018

View reviewed changes

packages/core/src/render3/jit/module.ts Outdated Show resolved Hide resolved

kara added 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 type: bug/fix labels Dec 4, 2018

JoostK reviewed Dec 4, 2018

View reviewed changes

benlesh added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Dec 5, 2018

feat(ivy): Forward refs now supported

a39e019

Adds deferred execution of scope setting for modules such that forward refs can be supported in ivy. Design docs can be found at https://docs.google.com/document/d/11KTbybis9rt0cZgMKd1wo_IKb6y1PMU-RoTDVLTXK4Y/edit#

IgorMinar force-pushed the ivy-jit-forwardref-support branch from a2d42c1 to a39e019 Compare December 5, 2018 17:53

IgorMinar approved these changes Dec 5, 2018

View reviewed changes

IgorMinar closed this in cd85832 Dec 5, 2018

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): Forward refs now supported #27439

fix(ivy): Forward refs now supported #27439

benlesh commented Dec 3, 2018

mary-poppins commented Dec 3, 2018

alxhub left a comment

alxhub Dec 3, 2018

benlesh Dec 3, 2018

alxhub Dec 3, 2018

benlesh Dec 3, 2018

alxhub Dec 3, 2018

benlesh Dec 3, 2018

JoostK Dec 3, 2018

alxhub Dec 3, 2018

benlesh Dec 3, 2018

alxhub Dec 3, 2018

benlesh Dec 3, 2018

benlesh commented Dec 3, 2018

mary-poppins commented Dec 3, 2018

googlebot commented Dec 4, 2018

googlebot commented Dec 4, 2018

benlesh commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

JoostK Dec 4, 2018

benlesh Dec 4, 2018

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 5, 2018

ngbot bot commented Dec 5, 2018

IgorMinar commented Dec 5, 2018

mary-poppins commented Dec 5, 2018

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

		@@ -19,14 +20,46 @@ import {reflectDependencies} from './util';

		const EMPTY_ARRAY: Type<any>[] = [];

		const moduleQueue: Array<{moduleType: Type<any>, ngModule: NgModule}> = [];

fix(ivy): Forward refs now supported #27439

fix(ivy): Forward refs now supported #27439

Conversation

benlesh commented Dec 3, 2018

mary-poppins commented Dec 3, 2018

alxhub 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh commented Dec 3, 2018

mary-poppins commented Dec 3, 2018

googlebot commented Dec 4, 2018

googlebot commented Dec 4, 2018

benlesh commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 4, 2018

mary-poppins commented Dec 5, 2018

ngbot bot commented Dec 5, 2018

IgorMinar commented Dec 5, 2018

mary-poppins commented Dec 5, 2018

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