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(zone.js): zone-node only patch Promise.prototype.then #49144
Conversation
abf4418
to
d1fdd70
Compare
d1fdd70
to
129e17b
Compare
86957e4
to
4499b3b
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, confirmed that this solved the issue.
4499b3b
to
3fe5b40
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
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.
@JiaLiPassion thanks for the fix 👍
I've left a couple minor comments and I also have a question (just to double check): would that version work correctly with the NodeJS version range that we have for Angular apps (starting from v15.2)?
(we can also update the feat(zone.js)
-> fix(zone.js)
, so that we can land this change sooner than v16)
Close angular#47872 zone-node only patches `Promise.prototype.then` instead of patch `Promise` itself. So the new NodeJS `SafePromise` will not complain the Promise.prototype.then called on incompatible receiver. We should also do this change on browser zone.js patch, but it will be a big breaking change, because Promise.prototype.then will not work with `fakeAsync` any longer.
@AndrewKushnir, thank you for the review, I have updated the code and also the git comment for the commit. |
3fe5b40
to
6c7754c
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.
Thanks for addressing the feedback @JiaLiPassion 👍
Caretaker note: this CL contains an updated patch for g3. |
This PR was merged into the repository by commit d1ac3aa. |
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. |
Close #47872
zone-node only patches
Promise.prototype.then
instead of patchPromise
itself. So the new NodeJSSafePromise
will not complainthe Promise.prototype.then called on incompatible receiver.
We should also do this change on browser zone.js patch, but it will
be a big breaking change, because Promise.prototype.then will not work
with
fakeAsync
any longer.