Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Staticaly downgraded components no longer rendered in AngularJS tests after call to $compile after upgrading @angular/upgrade from 7.1.4 to 7.2.0 #30330

Closed
brannislav opened this issue May 8, 2019 · 16 comments

Comments

@brannislav
Copy link

commented May 8, 2019

馃悶 bug report

Affected Package

The issue is caused by package @angular/upgrade: 7.2.0

Is this a regression?

Yes, the previous version in which this bug was not present was: 7.1.4

Description

A clear and concise description of the problem...

Our existing AngularJS tests started failing after upgrading @angular\upgrade from 7.1.4 to 7.2.0. The reason for the failures is that downgraded components are no-longer rendered in AngularJS tests after calling $compile.
The cause seems to be the introduction of Promise.all at bc0ee01#diff-fbe3a53baaa7090c3afd629acfd73fccR207

馃敩 Minimal Reproduction

It will be very hard to create minimal reproduction environment due to the co-existence of angular & AngularJS in the app and considering our complex setup.
In a nutshell we use $compile to compile templates containing downgraded components and than we call $scope.$apply().
After that we perform some verification of the rendered html. It seems that after this change doDowngrade is called well after the verification completes. In a sense this change is breaking the synchronous nature of Angular 1's $compile as stated in the jsdoc of ParentInjectorPromise.

We are using UpgradeAppType.Static. In our case finalParentInjector & finalModuleInjector are the same injector so I did an experiment and reverted back to having parentInjector.then(downgradeFn); and all tests started passing again.

馃敟 Exception or Error





馃實 Your Environment

Angular Version:




     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / 鈻 \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/


Angular CLI: 7.3.7
Node: 8.11.1
OS: win32 x64
Angular: 7.2.14
... animations, common, compiler, compiler-cli, core, forms
... http, language-service, platform-browser
... platform-browser-dynamic, router, upgrade

Package                            Version
------------------------------------------------------------
@angular-devkit/architect          0.13.7
@angular-devkit/build-angular      0.13.8
@angular-devkit/build-ng-packagr   0.13.8
@angular-devkit/build-optimizer    0.13.8
@angular-devkit/build-webpack      0.13.8
@angular-devkit/core               7.3.7
@angular-devkit/schematics         7.3.7
@angular/cli                       7.3.7
@ngtools/json-schema               1.1.0
@ngtools/webpack                   7.3.8
@schematics/angular                7.3.7
@schematics/update                 0.13.7
ng-packagr                         4.7.1
rxjs                               6.2.2
typescript                         3.2.4
webpack                            4.29.0


Anything else relevant?

@gkalpak

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Thx, @brannislav. LMK if you are interested in working on the fix (I can help/guide you with that), so we can coordinate our efforts better.

@gkalpak

This comment has been minimized.

Copy link
Member

commented May 10, 2019

One possible fix is to expand ParentInjectorPromise (which is basically a stripped-down, synchronous Promise implementation) to have a static .all() method, which would also preserve the synchronicity (if possible), and then replace Promise.all() with ParentInjectorPromise.all().

This would be synchronous if both injectors are either Injector instances or synchronously resolved ParentInjectorPromise instances or asynchronously otherwise.

(@brannislav, could you verify that this would solve the issue for you?)

First, we would probably need to have a more generic SyncPromise class (and extend it for ParentInjectorPromise):

class SyncPromise<T> {
  // TODO(issue/24571): remove '!'.
  protected value !: T;
  private resolved = false;
  private callbacks: ((value: T) => unknown)[] = [];

  resolve(value: T): void {
    this.value = value;
    this.resolved = true;

    // Run the queued callbacks.
    this.callbacks.forEach(callback => callback(value));
    this.callbacks.length = 0;
  }

  then(callback: (value: T) => unknown): void {
    if (this.resolved) {
      callback(this.value);
    } else {
      this.callbacks.push(callback);
    }
  }
}

class ParentInjectorPromise extends SyncPromise<Injector> {
  private injectorKey: string = controllerKey(INJECTOR_KEY);

  constructor(private element: IAugmentedJQuery) {
    super();

    // Store the promise on the element.
    element.data !(this.injectorKey, this);
  }

  resolve(injector: Injector): void {
    // Store the real injector on the element.
    this.element.data !(this.injectorKey, injector);

    // Release the element to prevent memory leaks.
    this.element = null !;

    // Resolve the promise.
    super.resolve(injector);
  }
}

Then, we could add a static .all() method to SyncPromise:

class SyncPromise<T> {
  ...
  static all<T>(promises: (T|Thenable<T>)[]): SyncPromise<T[]> {
    const aggrPromise = new SyncPromise<T[]>();

    let resolvedCount = 0;
    const results: T[] = [];    
    const resolve = (idx: number, value: T) => {
      results[idx] = value;
      if (++resolvedCount === promises.length) aggrPromise.resolve(results);
    };
    
    promises.forEach((p, i) => {
      if (p && isThenable(p as object)) {
        p.then(v => resolve(i, v));
      } else {
        resolve(i, p);
      }
    });
    
    return aggrPromise;
  }
  ...
}

(This is a minimal, quick implementation for demonstration purposes. It might have bugs and might be missing pieces (e.g. error handling).)

artem-galas added a commit to artem-galas/angular that referenced this issue May 27, 2019

fix(upgrade): implement all method for ParentInjectorPromise
Create base class SyncPromise which represents synchronous promise implemintation.
Implement `all` static method.

Closes angular#30330
@artem-galas

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

@gkalpak
I would like to try solve this issue.

I just copy/paste your suggestion, and I want to test it. But I didn't find any test file that test ParentInjectorPromise class.

Could you please give me a clue how to test it? Or we should test it manually (create AngularJS app -> downgrade some component, and so on)

Thanks.

@gkalpak

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Thx for looking into this, @artem-galas 鉂わ笍

There are indeed no tests for ParentInjectorPromise itself (it is tested indirectly as part of the downgradeComponent() unit tests). Most ngUpgrade unit tests involve "mini-apps", so you should be able to reproduce @brannislav's failure in such a test. (LMK if you have trouble with that.)

Since you brought it up, it might be a good idea to add some unit tests for ParentInjectorPromise/SyncPromise now that their logic is not trivial 馃槈

artem-galas added a commit to artem-galas/angular that referenced this issue May 29, 2019

fix(upgrade): implement all method for ParentInjectorPromise
Create base class SyncPromise which represents synchronous promise implemintation.
Implement `all` static method.

Closes angular#30330

artem-galas added a commit to artem-galas/angular that referenced this issue May 30, 2019

fix(upgrade): implement all method for ParentInjectorPromise
Create base class SyncPromise which represents synchronous promise implemintation.
Implement `all` static method.

Closes angular#30330
@artem-galas

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

I tried to reproduce error in tests, but seems I do something wrong.

I create Ng2Component and Ng2Module. Then create ng1Module where I create directives with downgradeComponent, and call $scope.$apply(); there.

@gkalpak Could you please look at test and give me piece of advice?

Actually it throw an error, but I'm not sure is it a right error:

HeadlessChrome 74.0.3729 (Mac OS X 10.13.6) [AngularJS v1.5] downgrade ng2 component should throw Promise.all error FAILED
	Error: [$rootScope:inprog] http://errors.angularjs.org/1.5.11/$rootScope/inprog?p0=%24apply
	    at npm/node_modules/angular-1.5/angular.min.js:6:426
	    at n (node_modules/angular-1.5/angular.min.js:139:271)
	    at m.$digest (node_modules/angular-1.5/angular.min.js:143:500)
	    at m.$apply (node_modules/angular-1.5/angular.min.js:147:363)
	    at Object.link (packages/upgrade/static/test/integration/downgrade_component_spec.ts:850:36 <- angular/packages/upgrade/static/test/integration/downgrade_component_spec.js:898:40)
	    at npm/node_modules/angular-1.5/angular.min.js:16:71
	    at sa (node_modules/angular-1.5/angular.min.js:82:244)
	    at n (node_modules/angular-1.5/angular.min.js:68:6)
	    at g (node_modules/angular-1.5/angular.min.js:59:428)
	    at npm/node_modules/angular-1.5/angular.min.js:59:67
	error properties: Object({ rejection: Error: [$rootScope:inprog] http://errors.angularjs.org/1.5.11/$rootScope/inprog?p0=%24apply, promise: [object Promise], zone: Zone({ _parent: Zone({ _parent: null, _name: '<root>', _properties: Object({  }), _zoneDelegate: ZoneDelegate({ _taskCounts: Object({ microTask: 0, macroTask: 0, eventTask: 0 }), zone: <circular reference: Object>, _parentDelegate: null, _forkZS: null, _forkDlgt: null, _forkCurrZone: null, _interceptZS: null, _interceptDlgt: null, _interceptCurrZone: null, _invokeZS: null, _invokeDlgt: null, _invokeCurrZone: null, _handleErrorZS: null, _handleErrorDlgt: null, _handleErrorCurrZone: null, _scheduleTaskZS: null, _scheduleTaskDlgt: null, _scheduleTaskCurrZone: null, _invokeTaskZS: null, _invokeTaskDlgt: null, _invokeTaskCurrZone: null, _cancelTaskZS: null, _cancelTaskDlgt: null, _cancelTaskCurrZone: null, _hasTaskZS: null, _hasTaskDlgt: null, _hasTaskDlgtOwner: null, _hasTaskCurrZone: null }) }), _name: 'ProxyZone', _properties: Object({ Prox ...
	Error: Uncaught (in promise): Error: [$rootScope:inprog] http://errors.angularjs.org/1.5.11/$rootScope/inprog?p0=%24apply
	    at resolvePromise (node_modules/zone.js/dist/zone.js:852:31)
	    at resolvePromise (node_modules/zone.js/dist/zone.js:809:17)
	    at eval (node_modules/zone.js/dist/zone.js:913:17)
	    at ZoneDelegate.invokeTask (node_modules/zone.js/dist/zone.js:423:31)
	    at AsyncTestZoneSpec.onInvokeTask (node_modules/zone.js/dist/zone-testing.js:754:25)
	    at ProxyZoneSpec.onInvokeTask (node_modules/zone.js/dist/zone-testing.js:336:39)
	    at ZoneDelegate.invokeTask (node_modules/zone.js/dist/zone.js:422:60)
	    at Zone.runTask (node_modules/zone.js/dist/zone.js:195:47)
	    at drainMicroTaskQueue (node_modules/zone.js/dist/zone.js:601:35)
@gkalpak

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

I tried to write a test for this and I could indeed not reproduce the issue. Looking at the code more closely, I see that finalParentInjector/finalModuleInjector can be either (a) an actual Promise (in which case the current code is fine) or (b) an Injector instance (in which case the current code is fine) or (c) a ParentInjectorPromise (which is what supposedly cases the issue).

In case (c), while ParentPromiseInjector is not resolved, then it should indeed act as a Promise, so the current code is fine too. And as soon as it is resolved, we replace the data on the element with the actual Injector instance, so child descendants should be able to grad the instances directly.

So, I am not sure under what circumstances the problem would mnifest itself.
@brannislav, could you share more details about the failure you are seeing (e.g. post the test code that is failing for you)?

@gkalpak

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

/ping @brannislav

@brannislav

This comment has been minimized.

Copy link
Author

commented Jun 10, 2019

I had to strip down most of the details because the code-base is proprietary.

The test code looks similar to:

beforeEach(function () {
   this.injectDependencies(
         '$rootScope',
         '$compile',
         'i18nService'
   );
   this.$scope = $rootScope.$new();
   var templateToCompile = $templateCache.get(templateUrl);
   var linkedElement = $compile(templateToCompile)(this.$scope);
   this.view = appendToDom.call(this, linkedElement);
   this.$scope.$apply();
});

it('displays the correct stack block content', function () {
   var localizedText = this.i18nService.getString('localizationKey');
   expect(this.view.find('clr-stack-block clr-stack-content span').text()).toBe(localizedText);
});

The template is essentially:

<div ng-controller="MyController as ctrl" class="settingsBlock">
   <clr-stack-view class="clr-component">
      <clr-stack-block>
         <clr-stack-label>{{::i18n('localizationKey')}}
         </clr-stack-label>
         <clr-stack-content>
            <span>{{ctrl.details}}</span>
         </clr-stack-content>
      </clr-stack-block>
   </clr-stack-view>
</div>

The downgrade of components is done in the main.ts (special one used only in tests Angular environment configuration):

export const clarityComponentsList: Array<[string, Type<any>]> = [
   ["clrStackView", ClrStackView],
   ["clrStackBlock", ClrStackBlock]
];

for (const [componentName, downgradePlugin] of clarityComponentsList) {
  ng1AppModule.directive(componentName, downgradeComponent({
	 component: downgradePlugin, propagateDigest: true
  }));
}

The downgraded components are from https://vmware.github.io/clarity/documentation/v0.13/stack-view

With "@angular/upgrade": 7.1.4 the downgraded components are rendered fine. With 7.2.0 the component is not rendered at the end of the beforeEach. Wrapping the test case in fakeAsync and explicitly calling tick() before the verification fixes the issue. Debugging the code that was changed in bc0ee01#diff-fbe3a53baaa7090c3afd629acfd73fccR207 showed that in our case finalParentInjector & finalModuleInjector are the same injector so I did an experiment and reverted back to having parentInjector.then(downgradeFn); and all tests started passing again.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

That is strange. Can you check what finalParentInjector/finalModuleInjector is set to and whether their .then() methods run the callback synchronously?

@brannislav

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

I managed to debug it today (attached screenshot below):

finalParentInjector

finalParentInjector seems to be instance of ParentInjectorPromise. Promise.all invokes ZoneAwarePromise.all which in turn calls ParentInjectorPromise.then.

ZoneAwarePromise

@gkalpak

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@brannislav, that is expected as long as finalParentInjector (which is a ParentInjectorPromise instance) is not resolved.
Based on your screenshots, it doesn't seem to be resolved (i.e. finalParentInjector.injector is undefined), so it is expected (and unavoidable) that the operation is async.

Maybe it is not the asynchronicity per se that is causing issues, but the fact that Promise.all() involves a microtick, while ParentInjectorPromise.then() would run the callbacks asap.

@brannislav, can you, please, try patching Angular to include the code from #30330 (comment) and verify that it solves the issue, so that we at least know we are not barking under the wrong tree 馃檹

@brannislav

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

@gkalpak I applied the patch verbatim and I can confirm all tests in our suite are back to green.
Indeed this seems to be an issue in Promise.all/ZoneAwarePromise.all.

@VlasovSergey

This comment has been minimized.

Copy link

commented Jul 18, 2019

We have a similar issue and patching angular/upgrade with replacing Promise.all() to sync version helped also.
Will it be fixed soon?

@gkalpak

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

I am having a hard time coming up with a test for this. I'll poke at it some more this week, but I think we should land the fix with or without a test.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

OK, I was able to create a minimal reproduction, based on #30330 (comment) (thx @brannislav). The issue happens when there are two "top-level" downgraded components; one projected into the other.

Now, on to writing a test 馃槂
@artem-galas, are you still interested in working on this or should someone else take over?

gkalpak added a commit to gkalpak/angular that referenced this issue Jul 25, 2019

fix(upgrade): compile downgraded components synchronously (if possible)
AngularJS compilation is a synchronous operation (unless having to fetch
a template, which is not supported for downgraded components).
Previously, ngUpgrade tried to retain the synchronous nature of the
compilation for downgraded components (when possible), by using a
synchronous thenable implementation (`ParentInjectorPromise`). This was
accidentally broken in angular#27217 by replacing a call to
`ParentInjectorPromise#then()` (which can be synchronous) with a call to
`Promise.all()` (which is asynchronous).

This commit fixes this by introducing a `SyncPromise.all()` static
method; similar to `Promise.all()` but retaining the synchronous
capabilities of `SyncPromise` (which `ParentInjectorPromise` inherits
from).

Fixes angular#30330
@gkalpak

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

I went ahead and submitted #31840 馃懠

@alxhub alxhub closed this in c1ae612 Aug 1, 2019

alxhub added a commit that referenced this issue Aug 1, 2019

fix(upgrade): compile downgraded components synchronously (if possibl鈥
鈥) (#31840)

AngularJS compilation is a synchronous operation (unless having to fetch
a template, which is not supported for downgraded components).
Previously, ngUpgrade tried to retain the synchronous nature of the
compilation for downgraded components (when possible), by using a
synchronous thenable implementation (`ParentInjectorPromise`). This was
accidentally broken in #27217 by replacing a call to
`ParentInjectorPromise#then()` (which can be synchronous) with a call to
`Promise.all()` (which is asynchronous).

This commit fixes this by introducing a `SyncPromise.all()` static
method; similar to `Promise.all()` but retaining the synchronous
capabilities of `SyncPromise` (which `ParentInjectorPromise` inherits
from).

Fixes #30330

PR Close #31840
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.