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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(upgrade): detect async downgrade component changes #14039
fix(upgrade): detect async downgrade component changes #14039
Conversation
097e445
to
0d8b699
Compare
This commit effectively reverts 7e0f02f but for `upgrade/static` as it was an invalid fix for angular#6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix angular#6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. See angular#13812
0d8b699
to
b1275ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
() => this.ngZone.runOutsideAngular(() => $rootScope.$evalAsync())); | ||
// We need to do this in the next tick so that we don't prevent the bootup | ||
// stabilizing | ||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be possible that we fail to propagate a change that happen in a microtask which completes after the $digest
invoked from angular.bootstrap
, but before we subscribe to onMicrotaskEmpty
.
For example, I would expect a test like the following to fail:
it('should propagate changes made asynchronously during bootstrap', fakeAsync(() => {
const ng1Component: angular.IComponent = {
template: '{{ $ctrl.value }}',
bindings: {value: '<'}
};
@Directive({selector: 'ng1'})
class Ng1ComponentFacade extends UpgradeComponent {
@Input() value: string;
constructor(elementRef: ElementRef, injector: Injector) {
super('ng1', elementRef, injector);
}
}
@Component({selector: 'ng2', template: '<ng1 [value]="value"></ng1>'})
class Ng2Component {
value: string;
constructor() {
Promise.resolve().then(() => this.value = 'foo');
}
}
@NgModule({
declarations: [Ng2Component, Ng1ComponentFacade],
entryComponents: [Ng2Component],
imports: [BrowserModule, UpgradeModule]
})
class Ng2Module {
ngDoBootstrap() {}
}
const ng1Module = angular.module('ng1', [])
.component('ng1', ng1Component)
.directive('ng2', downgradeComponent({component: Ng2Component}));
const element = html('<ng2></ng2>');
bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(upgrade => {
tick();
expect(element.textContent.trim()).toBe('foo');
});
}));
UPDATE:
Actually, it turns out the test passes, because there is "stuff" that goes on during the bootstrap phase that cause onMicrotaskEmpty
to emit. So, we'll be fine 😃
This commit effectively reverts 7e0f02f but for `upgrade/static` as it was an invalid fix for #6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix #6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. See #13812 PR Close #14039
This commit effectively reverts 7e0f02f but for `upgrade/static` as it was an invalid fix for angular#6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix angular#6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. See angular#13812 PR Close angular#14039
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This commit effectively reverts 7e0f02f but for
upgrade/static
as it was an invalid fix for #6385, that created a more significant
bug, which was that changes were not always being detected.
Angular 1 digests should be run inside the ngZone to ensure
that async changes are detected.
We don't know how to fix #6385 without breaking change detection
at this stage. That issue is triggered by async operations, such as
setTimeout
, being triggered inside scope watcher functions.One could argue that watcher functions should be pure and not do
work such as triggering async operations. It is possible that the
original use case could be supported by moving the debounce
logic into the watch listener function, which is only called if the
watched value actually changes.
See #13812
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: