GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix(zone.js): zone-node only patch Promise.prototype.then by JiaLiPassion · Pull Request #49144 · angular/angular

Conversation

@JiaLiPassion

@JiaLiPassion JiaLiPassion commented Feb 20, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Close #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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brandonroberts, alan-agius4, AndrewKushnir, and nitedani reacted with hooray emoji
All reactions
  • 🎉 4 reactions
@JiaLiPassion JiaLiPassion added the state: WIP label Feb 20, 2023
@JiaLiPassion
JiaLiPassion marked this pull request as draft February 20, 2023 22:38
@angular-robot angular-robot Bot added the detected: feature PR contains a feature commit label Feb 20, 2023
@JiaLiPassion
JiaLiPassion force-pushed the test-zone-promise branch 2 times, most recently from abf4418 to d1fdd70 Compare February 21, 2023 04:25
@JiaLiPassion
JiaLiPassion marked this pull request as ready for review February 21, 2023 05:05
@pullapprove
pullapprove Bot requested a review from josephperrott February 21, 2023 05:05
@JiaLiPassion JiaLiPassion added area: zones Issues related to zone.js and removed state: WIP labels Feb 21, 2023
@ngbot ngbot Bot modified the milestone: Backlog Feb 21, 2023
@JiaLiPassion
JiaLiPassion requested a review from alan-agius4 February 21, 2023 05:06
@JiaLiPassion
JiaLiPassion force-pushed the test-zone-promise branch from d1fdd70 to 129e17b Compare February 21, 2023 05:47
@JeanMeche JeanMeche mentioned this pull request Feb 21, 2023
[BUG] Node > 16.17.1: Method Promise.prototype.then called on incompatible receiver analogjs/analog#203
Closed
2 tasks
@JiaLiPassion
JiaLiPassion force-pushed the test-zone-promise branch 3 times, most recently from 86957e4 to 4499b3b Compare February 21, 2023 12:26
alan-agius4
alan-agius4 reviewed Feb 21, 2023
View reviewed changes
Comment thread tools/testing/node_tests.init.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
alan-agius4
alan-agius4 approved these changes Feb 21, 2023
View reviewed changes

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@alan-agius4 alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 21, 2023
@JiaLiPassion
JiaLiPassion force-pushed the test-zone-promise branch from 4499b3b to 3fe5b40 Compare February 21, 2023 13:02
@JiaLiPassion
JiaLiPassion requested review from alxhub and removed request for josephperrott February 21, 2023 13:02
@pullapprove
pullapprove Bot requested a review from josephperrott February 21, 2023 13:03
josephperrott
josephperrott approved these changes Feb 21, 2023
View reviewed changes

@josephperrott josephperrott left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 21, 2023
AndrewKushnir
AndrewKushnir approved these changes Feb 21, 2023
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/zone.js/lib/node/promise.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread packages/zone.js/lib/node/promise.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
@JiaLiPassion
fix(zone.js): zone-node only patch Promise.prototype.then
6c7754c 
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.
@JiaLiPassion

JiaLiPassion commented Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

@AndrewKushnir, thank you for the review, I have updated the code and also the git comment for the commit.
AndrewKushnir reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JiaLiPassion
JiaLiPassion force-pushed the test-zone-promise branch from 3fe5b40 to 6c7754c Compare February 21, 2023 23:38
@angular-robot angular-robot Bot removed the detected: feature PR contains a feature commit label Feb 21, 2023
@AndrewKushnir AndrewKushnir changed the title feat(zone.js): zone-node only patch Promise.prototype.then fix(zone.js): zone-node only patch Promise.prototype.then Feb 22, 2023
AndrewKushnir
AndrewKushnir approved these changes Feb 22, 2023
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

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 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit target: patch This PR is targeted for the next patch release and removed action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release labels Feb 22, 2023
@AndrewKushnir

AndrewKushnir commented Feb 22, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Presubmit (+ TGP).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AndrewKushnir

AndrewKushnir commented Feb 22, 2023

Copy link
Copy Markdown
Contributor

Caretaker note: this CL contains an updated patch for g3.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 24, 2023
alxhub
alxhub approved these changes Feb 24, 2023
View reviewed changes
@alan-agius4 alan-agius4 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 and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Feb 24, 2023
@AndrewKushnir AndrewKushnir added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Feb 27, 2023
@AndrewKushnir

AndrewKushnir commented Feb 27, 2023

Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit d1ac3aa.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AndrewKushnir AndrewKushnir closed this in d1ac3aa Feb 27, 2023
@JiaLiPassion JiaLiPassion mentioned this pull request Feb 28, 2023
release: cut the zone.js-0.13 release #49240
Closed
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Mar 30, 2023

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Mar 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alxhub alxhub alxhub approved these changes
@josephperrott josephperrott josephperrott approved these changes
@alan-agius4 alan-agius4 alan-agius4 approved these changes
@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: zones Issues related to zone.js merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

[zone.js] Node > 16.17.1: Method Promise.prototype.then called on incompatible receiver

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@JiaLiPassion @AndrewKushnir @alxhub @josephperrott @alan-agius4
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.