Skip to content

fix(zone.js): should ignore multiple resolve call#45283

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:promise-multiple-resolve
Closed

fix(zone.js): should ignore multiple resolve call#45283
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:promise-multiple-resolve

Conversation

@JiaLiPassion
Copy link
Contributor

Close #44913

The following case is not handled correctly by zone.js.

const delayedPromise = new Promise((resolve) => {
  setTimeout(resolve, 1, 'timeout');
});

new Promise((resolve) => {
  resolve(delayedPromise);
  resolve('second call');
}).then(console.log);

It should output timeout, since the promise is resolved by the
1st resolve, the second call should be ignored.

So this is a bug that the original implementation not ensure the
resolve is only called once.

Close angular#44913

The following case is not handled correctly by `zone.js`.
```
const delayedPromise = new Promise((resolve) => {
  setTimeout(resolve, 1, 'timeout');
});

new Promise((resolve) => {
  resolve(delayedPromise);
  resolve('second call');
}).then(console.log);
```

It should output `timeout`, since the promise is resolved by the
1st resolve, the `second call` should be ignored.

So this is a bug that the original implementation not ensure the
`resolve` is only called once.
@JiaLiPassion JiaLiPassion added the area: zones Issues related to zone.js label Mar 7, 2022
@JiaLiPassion JiaLiPassion requested a review from gkalpak March 7, 2022 09:27
@ngbot ngbot bot added this to the Backlog milestone Mar 7, 2022
@JiaLiPassion JiaLiPassion added target: minor This PR is targeted for the next minor release target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Mar 7, 2022
@dylhunn dylhunn self-requested a review March 26, 2022 00:23
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really interesting bug!

@dylhunn
Copy link
Contributor

dylhunn commented Mar 26, 2022

I am sort of surprised that this is well-defined behavior, i.e. all non-zone environments always print timeout.

@dylhunn
Copy link
Contributor

dylhunn commented Mar 26, 2022

@JiaLiPassion explained to me on chat that the spec guarantees timeout will be printed. Also, he says this is merge ready.

@dylhunn dylhunn added the action: merge The PR is ready for merge by the caretaker label Mar 26, 2022
@dylhunn
Copy link
Contributor

dylhunn commented Mar 26, 2022

This PR was merged into the repository by commit aebf165.

dylhunn pushed a commit that referenced this pull request Mar 26, 2022
Close #44913

The following case is not handled correctly by `zone.js`.
```
const delayedPromise = new Promise((resolve) => {
  setTimeout(resolve, 1, 'timeout');
});

new Promise((resolve) => {
  resolve(delayedPromise);
  resolve('second call');
}).then(console.log);
```

It should output `timeout`, since the promise is resolved by the
1st resolve, the `second call` should be ignored.

So this is a bug that the original implementation not ensure the
`resolve` is only called once.

PR Close #45283
@dylhunn dylhunn closed this in aebf165 Mar 26, 2022
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: zones Issues related to zone.js target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resolve method in zone.js Promise does not work correctly

2 participants