Skip to content

Commit

Permalink
fix(zone.js): Promise.resolve(subPromise) should return subPromise (#…
Browse files Browse the repository at this point in the history
…53423)

In the original `Promise` impelmentation, zone.js follow the spec from
https://promisesaplus.com/#point-51.

```
const p1 = Promise.resolve(1);
const p2 = Promise.resolve(p1);

p1 === p2; // false
```
in this case, `p2` should be the same status with `p1` but they are
still different instances.

And for some edge case.

```
class MyPromise extends Promise {
  constructor(sub) {
    super((res) => res(null));
    this.sub = sub;
  }
  then(onFufilled, onRejected) {
    this.sub.then(onFufilled, onRejected);
  }
}

const p1 = new Promise(setTimeout(res), 100);
const myP = new MyPromise(p1);
const r = await myP;
r === 1; // false
```

So in the above code, `myP` is not the same instance with `p1`,
and since `myP` is resolved in constructor, so `await myP` will
just pass without waiting for `p1`.

And in the current `tc39` spec here https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-promise-resolve
`Promise.resolve(subP)` should return `subP`.

```
const p1 = Promise.resolve(1);
const p2 = Promise.resolve(p1);

p1 === p2; // true
```

So the above `MyPromise` can wait for the `p1` correctly.

PR Close #53423
  • Loading branch information
JiaLiPassion authored and alxhub committed Dec 11, 2023
1 parent e3a6bf9 commit 08b0c87
Show file tree
Hide file tree
Showing 7 changed files with 30 additions and 312 deletions.
1 change: 0 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,6 @@ jobs:
# Install
- run: yarn --cwd packages/zone.js install --frozen-lockfile --non-interactive
# Run zone.js tools tests
- run: yarn --cwd packages/zone.js promisetest
- run: yarn --cwd packages/zone.js promisefinallytest
- run: yarn --cwd packages/zone.js jest:test
- run: yarn --cwd packages/zone.js jest:nodetest
Expand Down
3 changes: 0 additions & 3 deletions packages/zone.js/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,5 @@ import 'zone.js/plugins/zone-patch-canvas';
|`zone-patch-user-media.js`|patch for `UserMedia API`|
|`zone-patch-message-port.js`|patch for `MessagePort API`|

## Promise A+ test passed
[![Promises/A+ 1.1 compliant](https://promisesaplus.com/assets/logo-small.png)](https://promisesaplus.com/)

## License
MIT
3 changes: 3 additions & 0 deletions packages/zone.js/lib/common/promise.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,9 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr
}

static resolve<R>(value: R): Promise<R> {
if (value instanceof ZoneAwarePromise) {
return value;
}
return resolvePromise(<ZoneAwarePromise<R>>new this(null as any), RESOLVED, value);
}

Expand Down
4 changes: 1 addition & 3 deletions packages/zone.js/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,13 @@
"jest-environment-jsdom": "^29.0.3",
"jest-environment-node": "^29.0.3",
"mocha": "^10.2.0",
"mock-require": "3.0.3",
"promises-aplus-tests": "^2.1.2"
"mock-require": "3.0.3"
},
"scripts": {
"closuretest": "./scripts/closure/closure_compiler.sh",
"electrontest": "cd test/extra && node electron.js",
"jest:test": "jest --config ./test/jest/jest.config.js ./test/jest/jest.spec.js",
"jest:nodetest": "jest --config ./test/jest/jest.node.config.js ./test/jest/jest.spec.js",
"promisetest": "node ./test/promise/promise-test.mjs",
"promisefinallytest": "mocha ./test/promise/promise.finally.spec.mjs"
},
"repository": {
Expand Down
25 changes: 24 additions & 1 deletion packages/zone.js/test/common/Promise.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {isNode, zoneSymbol} from '../../lib/common/utils';
import {isNode} from '../../lib/common/utils';
import {ifEnvSupports} from '../test-util';

declare const global: any;
Expand Down Expand Up @@ -76,6 +76,12 @@ describe(
expect(new Promise(() => null) instanceof Promise).toBe(true);
});

it('Promise.resolve(subPromise) should equal to subPromise', () => {
const p1 = Promise.resolve(1);
const p2 = Promise.resolve(p1);
expect(p1).toBe(p2);
});

xit('should ensure that Promise this is instanceof Promise', () => {
expect(() => {
Promise.call({} as any, () => null);
Expand Down Expand Up @@ -130,6 +136,23 @@ describe(
expect(new MyPromise(() => {}).then(() => null) instanceof MyPromise).toBe(true);
});

it('should allow subclassing with own then', (done: DoneFn) => {
class MyPromise extends Promise<any> {
constructor(private sub: Promise<any>) {
super((resolve) => {resolve(null)});
}

override then(onFulfilled: any, onRejected: any) {
return this.sub.then(onFulfilled, onRejected);
}
}
const p = Promise.resolve(1);
new MyPromise(p).then((v: any) => {
expect(v).toBe(1);
done();
}, () => done());
});

it('Symbol.species should return ZoneAwarePromise', () => {
const empty = function() {};
const promise = Promise.resolve(1);
Expand Down
11 changes: 0 additions & 11 deletions packages/zone.js/test/promise/promise-test.mjs

This file was deleted.

0 comments on commit 08b0c87

Please sign in to comment.