Skip to content

Commit

Permalink
fix(zone.js): patch Response methods returned by fetch (#50653)
Browse files Browse the repository at this point in the history
This commit updates the implementation of the `fetch` patch and additionally
patches `Response` methods which return promises. These are `arrayBuffer`, `blob`,
`formData`, `json` and `text`. This fixes the issue when zone becomes stable too early
before all of the `fetch` tasks complete. Given the following code:
```ts
appRef.isStable.subscribe(console.log);
fetch(...).then(response => response.json()).then(console.log);
```
The `isStable` observer would log `false, true, false, true`. This was happening because
`json()` was returning a native promise (and not a `ZoneAwarePromise`). But calling `then`
on the native promise returns a `ZoneAwarePromise` which notifies Angular about the task
being scheduled and forces to re-calculate the `isStable` state.

Issue: #50327

PR Close #50653
  • Loading branch information
arturovt authored and thePunderWoman committed Jan 31, 2024
1 parent f87f058 commit 260d3ed
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 41 deletions.
110 changes: 69 additions & 41 deletions packages/zone.js/lib/common/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,61 +26,89 @@ Zone.__load_patch('fetch', (global: any, Zone: ZoneType, api: _ZonePrivate) => {
const ZoneAwarePromise = global.Promise;
const symbolThenPatched = api.symbol('thenPatched');
const fetchTaskScheduling = api.symbol('fetchTaskScheduling');
const OriginalResponse = global.Response;
const placeholder = function() {};

const createFetchTask =
(source: string, data: TaskData|undefined, originalImpl: any, self: any, args: any[],
ac?: AbortController) => new Promise((resolve, reject) => {
const task = Zone.current.scheduleMacroTask(
source, placeholder, data,
() => {
// The promise object returned by the original implementation passed into the
// function. This might be a `fetch` promise, `Response.prototype.json` promise,
// etc.
let implPromise;
let zone = Zone.current;

try {
(zone as any)[fetchTaskScheduling] = true;
implPromise = originalImpl.apply(self, args);
} catch (error) {
reject(error);
return;
} finally {
(zone as any)[fetchTaskScheduling] = false;
}

if (!(implPromise instanceof ZoneAwarePromise)) {
let ctor = implPromise.constructor;
if (!ctor[symbolThenPatched]) {
api.patchThen(ctor);
}
}

implPromise.then(
(resource: any) => {
if (task.state !== 'notScheduled') {
task.invoke();
}
resolve(resource);
},
(error: any) => {
if (task.state !== 'notScheduled') {
task.invoke();
}
reject(error);
});
},
() => {
ac?.abort();
});
});

global['fetch'] = function() {
const args = Array.prototype.slice.call(arguments);
const options = args.length > 1 ? args[1] : {};
const signal = options && options.signal;
const signal: AbortSignal|undefined = options?.signal;
const ac = new AbortController();
const fetchSignal = ac.signal;
options.signal = fetchSignal;
args[1] = options;

if (signal) {
const nativeAddEventListener =
signal[Zone.__symbol__('addEventListener')] || signal.addEventListener;
signal[Zone.__symbol__('addEventListener') as 'addEventListener'] ||
signal.addEventListener;

nativeAddEventListener.call(signal, 'abort', function() {
ac!.abort();
}, {once: true});
}
return new Promise((res, rej) => {
const task = Zone.current.scheduleMacroTask(
'fetch', placeholder, {fetchArgs: args} as FetchTaskData,
() => {
let fetchPromise;
let zone = Zone.current;
try {
(zone as any)[fetchTaskScheduling] = true;
fetchPromise = fetch.apply(this, args);
} catch (error) {
rej(error);
return;
} finally {
(zone as any)[fetchTaskScheduling] = false;
}

if (!(fetchPromise instanceof ZoneAwarePromise)) {
let ctor = fetchPromise.constructor;
if (!ctor[symbolThenPatched]) {
api.patchThen(ctor);
}
}
fetchPromise.then(
(resource: any) => {
if (task.state !== 'notScheduled') {
task.invoke();
}
res(resource);
},
(error: any) => {
if (task.state !== 'notScheduled') {
task.invoke();
}
rej(error);
});
},
() => {
ac.abort();
});
});
return createFetchTask('fetch', {fetchArgs: args} as FetchTaskData, fetch, this, args, ac);
};

if (OriginalResponse?.prototype) {
// https://fetch.spec.whatwg.org/#body-mixin
['arrayBuffer', 'blob', 'formData', 'json', 'text']
// Safely check whether the method exists on the `Response` prototype before patching.
.filter(method => typeof OriginalResponse.prototype[method] === 'function')
.forEach(method => {
api.patchMethod(
OriginalResponse.prototype, method,
(delegate: Function) => (self, args) => createFetchTask(
`Response.${method}`, undefined, delegate, self, args, undefined));
});
}
});
33 changes: 33 additions & 0 deletions packages/zone.js/test/common/fetch.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,39 @@ describe(
});
});

// https://github.com/angular/angular/issues/50327
it('Response.json() should be considered as macroTask', done => {
fetchZone.run(() => {
global['fetch']('/base/angular/packages/zone.js/test/assets/sample.json')
.then((response: any) => {
const promise = response.json();
// Ensure it's a `ZoneAwarePromise`.
expect(promise).toBeInstanceOf(global.Promise);
return promise;
})
.then(() => {
expect(logs).toEqual([
'scheduleTask:fetch:macroTask', 'scheduleTask:Promise.then:microTask',
'invokeTask:Promise.then:microTask', 'invokeTask:fetch:macroTask',
'scheduleTask:Promise.then:microTask', 'invokeTask:Promise.then:microTask',
// Please refer to the issue link above. Previously, `Response` methods were not
// patched by zone.js, and their return values were considered only as
// microtasks (not macrotasks). The Angular zone stabilized prematurely,
// occurring before the resolution of the `response.json()` promise due to the
// falsy value of `zone.hasPendingMacrotasks`. We are now ensuring that
// `Response` methods are treated as macrotasks, similar to the behavior of
// `fetch`.
'scheduleTask:Response.json:macroTask', 'scheduleTask:Promise.then:microTask',
'invokeTask:Promise.then:microTask', 'invokeTask:Response.json:macroTask',
'scheduleTask:Promise.then:microTask', 'invokeTask:Promise.then:microTask',
'scheduleTask:Promise.then:microTask', 'invokeTask:Promise.then:microTask'
]);

done();
});
});
});

it('cancel fetch should invoke onCancelTask',
ifEnvSupportsWithDone('AbortController', (done: DoneFn) => {
if (isSafari()) {
Expand Down

0 comments on commit 260d3ed

Please sign in to comment.