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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(upgrade): upgrade Directive facade should not return different instance from constructor #27660

Closed
wants to merge 3 commits into
base: master
from

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Dec 13, 2018

In ngUpgrade (dynamic) we create a dynamic Angular Directive that wraps AngularJS components
that are being upgraded. The constructor of this Directive class returns a different instance
than this. It is this instance that actually contains the life-cycle hook handlers.

This would break in ivy, since the methods on the prototype of the original class are wired up,
rather than the instance methods. This results in hooks like ngOnInit not being called.

This PR refactors the code to extend the inner class that was being returned so that the
prototype chain is correct for both ViewEngine and ivy.

This change resolves a number of failing ivy tests, but also exposes other failures that were
masked by this issue. The tests have been updated accordingly.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 13, 2018

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:fw-812 branch from 1ef6aa4 to c096bf7 Dec 13, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 13, 2018

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:fw-812 branch from c096bf7 to b809816 Dec 14, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 14, 2018

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:fw-812 branch from b809816 to 4a1e843 Dec 14, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 14, 2018

@petebacondarwin petebacondarwin changed the title test(upgrade): fix FW-812 (WIP) fix(upgrade): upgrade Directive facade should not return different instance from constructor Dec 14, 2018

@petebacondarwin petebacondarwin requested review from mhevery and gkalpak Dec 14, 2018

petebacondarwin added some commits Dec 14, 2018

test(upgrade): update test failure cause descriptions
The changes here clarify that we now have a better description
of the root cause of the failure.
fix(upgrade): upgrade Directive facade should not return different in…
…stance from constructor

In ngUpgrade (dynamic) we create a dynamic Angular `Directive` that wraps AngularJS components
that are being upgraded. The constructor of this `Directive` class returns a different instance
than `this`. It is this instance that actually contains the life-cycle hook handlers.

This would break in ivy, since the methods on the prototype of the original class are wired up,
rather than the instance methods. This results in hooks like `ngOnInit` not being called.

This commit refactors the code to extend the inner class that was being returned so that the
prototype chain is correct for both ViewEngine and ivy.

This change resolves a number of failing ivy tests, but also exposes other failures that were
masked by this issue. The tests have been updated accordingly.

(FW-812)

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:fw-812 branch from 4a1e843 to 9cb16e0 Dec 14, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 14, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 14, 2018

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:fw-812 branch from 0ba061f to 1f7c9bf Dec 14, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 14, 2018

@mhevery

This comment has been minimized.

@mhevery mhevery closed this in 3444aee Dec 17, 2018

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

fix(upgrade): upgrade Directive facade should not return different in…
…stance from constructor (#27660)

In ngUpgrade (dynamic) we create a dynamic Angular `Directive` that wraps AngularJS components
that are being upgraded. The constructor of this `Directive` class returns a different instance
than `this`. It is this instance that actually contains the life-cycle hook handlers.

This would break in ivy, since the methods on the prototype of the original class are wired up,
rather than the instance methods. This results in hooks like `ngOnInit` not being called.

This commit refactors the code to extend the inner class that was being returned so that the
prototype chain is correct for both ViewEngine and ivy.

This change resolves a number of failing ivy tests, but also exposes other failures that were
masked by this issue. The tests have been updated accordingly.

(FW-812)

PR Close #27660

@petebacondarwin petebacondarwin deleted the petebacondarwin:fw-812 branch Dec 18, 2018

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

test(upgrade): update test failure cause descriptions (angular#27660)
The changes here clarify that we now have a better description
of the root cause of the failure.

PR Close angular#27660

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

fix(upgrade): upgrade Directive facade should not return different in…
…stance from constructor (angular#27660)

In ngUpgrade (dynamic) we create a dynamic Angular `Directive` that wraps AngularJS components
that are being upgraded. The constructor of this `Directive` class returns a different instance
than `this`. It is this instance that actually contains the life-cycle hook handlers.

This would break in ivy, since the methods on the prototype of the original class are wired up,
rather than the instance methods. This results in hooks like `ngOnInit` not being called.

This commit refactors the code to extend the inner class that was being returned so that the
prototype chain is correct for both ViewEngine and ivy.

This change resolves a number of failing ivy tests, but also exposes other failures that were
masked by this issue. The tests have been updated accordingly.

(FW-812)

PR Close angular#27660
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.