Skip to content
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

feat(core): Reimplement fetch source as async generator #3043

Merged
merged 31 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
8e4110f
Bump to wonka@^6.1.1
kitten Dec 1, 2022
c429c58
Reimplement makeFetchSource with internal async iterable
kitten Dec 1, 2022
39b7d4c
Add changeset
kitten Dec 1, 2022
b95b6d2
Add missing micro-tick delay and update tests
kitten Dec 1, 2022
12e7d97
Delete duplicated tests from multipartFetchExchange
kitten Dec 1, 2022
d124e29
Simplify fetchSource parameters
kitten Dec 1, 2022
65e6613
Update to wonka@^6.1.2 to fix takeUntil leak
kitten Dec 1, 2022
9a68eed
Remove unused, unsupported stream branch
kitten Dec 1, 2022
4184f07
Merge branch 'fix/node-memory-leak-fetch-source' into feat/fetch-sour…
kitten Mar 14, 2023
644ca3b
Update abort call
kitten Mar 14, 2023
4279817
Extract body to async iterator conversion
kitten Mar 14, 2023
69fdc4e
Remove assumed JSON header check
kitten Mar 14, 2023
432cb6c
Simplify boundary check
kitten Mar 14, 2023
9a551c0
Remove excessive boundary index check
kitten Mar 14, 2023
5928d23
Flush all results one-by-one
kitten Mar 14, 2023
9736d9e
Update outdated snapshot
kitten Mar 14, 2023
03cb685
Fix break condition
kitten Mar 14, 2023
7a61577
Remove manual redirect status check
kitten Mar 14, 2023
b1aa7ee
Replace slice with startsWith
kitten Mar 14, 2023
a1ddb49
Remove hasResults check
kitten Mar 14, 2023
fb671e3
Remove redundant response.headers check
kitten Mar 14, 2023
e109114
Update persisted/multipart/execute tests
kitten Mar 14, 2023
4f82b52
Simplify streamBody implementation
kitten Mar 14, 2023
044b870
Remove redundant slices and extract multipart chunking
kitten Mar 14, 2023
8ce67c3
Add closing signal if it's missing
kitten Mar 14, 2023
6903339
Add missing type annotation
kitten Mar 14, 2023
bc8b587
Throw JSON error if initial JSON is invalid
kitten Mar 14, 2023
eedc540
Add missing reader.cancel call
kitten Mar 14, 2023
b03abec
Rename protocol function
kitten Mar 14, 2023
fd8a9bf
Fix incorrectly placed next buffer code
kitten Mar 14, 2023
1698783
Add note to changeset
kitten Mar 14, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/real-donkeys-act.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@urql/core': patch
---

Replace fetch source implementation with async generator implementation, based on Wonka's `fromAsyncIterable`.
This also further hardens our support for the "Incremental Delivery" specification and
refactors its implementation and covers more edge cases.
1 change: 1 addition & 0 deletions exchanges/execute/src/execute.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ describe('on operation', () => {

fetchMock.mockResolvedValue({
status: 200,
headers: { get: () => 'application/json' },
text: vi
.fn()
.mockResolvedValue(JSON.stringify({ data: mockHttpResponseData })),
Expand Down
12 changes: 10 additions & 2 deletions exchanges/graphcache/src/offlineExchange.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ const storage = {

describe('storage', () => {
it('should read the metadata and dispatch operations on initialization', () => {
const client = createClient({ url: 'http://0.0.0.0' });
const client = createClient({
url: 'http://0.0.0.0',
exchanges: [],
});

const reexecuteOperation = vi
.spyOn(client, 'reexecuteOperation')
.mockImplementation(() => undefined);
Expand Down Expand Up @@ -100,7 +104,11 @@ describe('offline', () => {
it('should intercept errored mutations', () => {
const onlineSpy = vi.spyOn(navigator, 'onLine', 'get');

const client = createClient({ url: 'http://0.0.0.0' });
const client = createClient({
url: 'http://0.0.0.0',
exchanges: [],
});

const queryOp = client.createRequestOperation('query', {
key: 1,
query: queryOne,
Expand Down
60 changes: 6 additions & 54 deletions exchanges/multipart-fetch/src/multipartFetchExchange.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Client, OperationResult, makeOperation } from '@urql/core';
import { empty, fromValue, pipe, Source, subscribe, toPromise } from 'wonka';
import { Client, OperationResult } from '@urql/core';
import { empty, fromValue, pipe, Source, toPromise } from 'wonka';

import {
vi,
expect,
Expand All @@ -23,9 +24,6 @@ import {
const fetch = (global as any).fetch as Mock;
const abort = vi.fn();

const abortError = new Error();
abortError.name = 'AbortError';

beforeAll(() => {
(global as any).AbortController = function AbortController() {
this.signal = undefined;
Expand Down Expand Up @@ -61,6 +59,7 @@ describe('on success', () => {
beforeEach(() => {
fetch.mockResolvedValue({
status: 200,
headers: { get: () => 'application/json' },
text: vi.fn().mockResolvedValue(response),
});
});
Expand Down Expand Up @@ -130,6 +129,7 @@ describe('on error', () => {
beforeEach(() => {
fetch.mockResolvedValue({
status: 400,
headers: { get: () => 'application/json' },
text: vi.fn().mockResolvedValue('{}'),
});
});
Expand Down Expand Up @@ -165,6 +165,7 @@ describe('on error', () => {
it('ignores the error when a result is available', async () => {
fetch.mockResolvedValue({
status: 400,
headers: { get: () => 'application/json' },
text: vi.fn().mockResolvedValue(response),
});

Expand All @@ -177,52 +178,3 @@ describe('on error', () => {
expect(data.data).toEqual(JSON.parse(response).data);
});
});

describe('on teardown', () => {
const fail = () => {
expect(true).toEqual(false);
};

it('does not start the outgoing request on immediate teardowns', () => {
fetch.mockRejectedValueOnce(abortError);

const { unsubscribe } = pipe(
fromValue(queryOperation),
multipartFetchExchange(exchangeArgs),
subscribe(fail)
);

unsubscribe();
expect(fetch).toHaveBeenCalledTimes(0);
expect(abort).toHaveBeenCalledTimes(1);
});

it('aborts the outgoing request', async () => {
fetch.mockRejectedValueOnce(abortError);

const { unsubscribe } = pipe(
fromValue(queryOperation),
multipartFetchExchange(exchangeArgs),
subscribe(fail)
);

await Promise.resolve();

unsubscribe();
expect(fetch).toHaveBeenCalledTimes(1);
expect(abort).toHaveBeenCalledTimes(1);
});

it('does not call the query', () => {
pipe(
fromValue(
makeOperation('teardown', queryOperation, queryOperation.context)
),
multipartFetchExchange(exchangeArgs),
subscribe(fail)
);

expect(fetch).toHaveBeenCalledTimes(0);
expect(abort).toHaveBeenCalledTimes(0);
});
});
16 changes: 16 additions & 0 deletions exchanges/persisted-fetch/src/persistedFetchExchange.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ it('accepts successful persisted query responses', async () => {
});

fetch.mockResolvedValueOnce({
status: 200,
headers: { get: () => 'application/json' },
text: () => Promise.resolve(expected),
});

Expand Down Expand Up @@ -63,9 +65,13 @@ it('supports cache-miss persisted query errors', async () => {

fetch
.mockResolvedValueOnce({
status: 200,
headers: { get: () => 'application/json' },
text: () => Promise.resolve(expectedMiss),
})
.mockResolvedValueOnce({
status: 200,
headers: { get: () => 'application/json' },
text: () => Promise.resolve(expectedRetry),
});

Expand Down Expand Up @@ -94,9 +100,13 @@ it('supports GET exclusively for persisted queries', async () => {

fetch
.mockResolvedValueOnce({
status: 200,
headers: { get: () => 'application/json' },
text: () => Promise.resolve(expectedMiss),
})
.mockResolvedValueOnce({
status: 200,
headers: { get: () => 'application/json' },
text: () => Promise.resolve(expectedRetry),
});

Expand Down Expand Up @@ -127,12 +137,18 @@ it('supports unsupported persisted query errors', async () => {

fetch
.mockResolvedValueOnce({
status: 200,
headers: { get: () => 'application/json' },
text: () => Promise.resolve(expectedMiss),
})
.mockResolvedValueOnce({
status: 200,
headers: { get: () => 'application/json' },
text: () => Promise.resolve(expectedRetry),
})
.mockResolvedValueOnce({
status: 200,
headers: { get: () => 'application/json' },
text: () => Promise.resolve(expectedRetry),
});

Expand Down
17 changes: 13 additions & 4 deletions packages/core/src/exchanges/fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ describe('on success', () => {
beforeEach(() => {
fetch.mockResolvedValue({
status: 200,
headers: { get: () => 'application/json' },
text: vi.fn().mockResolvedValue(response),
});
});
Expand Down Expand Up @@ -91,6 +92,7 @@ describe('on error', () => {
beforeEach(() => {
fetch.mockResolvedValue({
status: 400,
headers: { get: () => 'application/json' },
text: vi.fn().mockResolvedValue(JSON.stringify({})),
});
});
Expand Down Expand Up @@ -126,6 +128,7 @@ describe('on error', () => {
it('ignores the error when a result is available', async () => {
fetch.mockResolvedValue({
status: 400,
headers: { get: () => 'application/json' },
text: vi.fn().mockResolvedValue(response),
});

Expand All @@ -143,8 +146,9 @@ describe('on teardown', () => {
const fail = () => {
expect(true).toEqual(false);
};

it('does not start the outgoing request on immediate teardowns', () => {
fetch.mockRejectedValueOnce(abortError);
fetch.mockResolvedValue(new Response('text', { status: 200 }));

const { unsubscribe } = pipe(
fromValue(queryOperation),
Expand All @@ -154,11 +158,11 @@ describe('on teardown', () => {

unsubscribe();
expect(fetch).toHaveBeenCalledTimes(0);
expect(abort).toHaveBeenCalledTimes(1);
expect(abort).toHaveBeenCalledTimes(0);
});

it('aborts the outgoing request', async () => {
fetch.mockRejectedValueOnce(abortError);
fetch.mockResolvedValue(new Response('text', { status: 200 }));

const { unsubscribe } = pipe(
fromValue(queryOperation),
Expand All @@ -169,11 +173,16 @@ describe('on teardown', () => {
await Promise.resolve();

unsubscribe();

// NOTE: We can only observe the async iterator's final run after a macro tick
await new Promise(resolve => setTimeout(resolve));
expect(fetch).toHaveBeenCalledTimes(1);
expect(abort).toHaveBeenCalledTimes(1);
expect(abort).toHaveBeenCalled();
});

it('does not call the query', () => {
fetch.mockResolvedValue(new Response('text', { status: 200 }));

pipe(
fromValue(
makeOperation('teardown', queryOperation, queryOperation.context)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,9 @@ exports[`on success > uses the mock fetch if given 1`] = `
{
"type": "return",
"value": {
"headers": {
"get": [Function],
},
"status": 200,
"text": [MockFunction spy] {
"calls": [
Expand Down
16 changes: 9 additions & 7 deletions packages/core/src/internal/fetchSource.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ import { makeOperation } from '../utils';
const fetch = (global as any).fetch as Mock;
const abort = vi.fn();

const abortError = new Error();
abortError.name = 'AbortError';

beforeAll(() => {
(global as any).AbortController = function AbortController() {
this.signal = undefined;
Expand Down Expand Up @@ -51,6 +48,7 @@ describe('on success', () => {
beforeEach(() => {
fetch.mockResolvedValue({
status: 200,
headers: { get: () => 'application/json' },
text: vi.fn().mockResolvedValue(response),
});
});
Expand All @@ -73,6 +71,7 @@ describe('on success', () => {
const fetchOptions = {};
const fetcher = vi.fn().mockResolvedValue({
status: 200,
headers: { get: () => 'application/json' },
text: vi.fn().mockResolvedValue(response),
});

Expand Down Expand Up @@ -102,6 +101,7 @@ describe('on error', () => {
fetch.mockResolvedValue({
status: 400,
statusText: 'Forbidden',
headers: { get: () => 'application/json' },
text: vi.fn().mockResolvedValue('{}'),
});
});
Expand Down Expand Up @@ -165,7 +165,7 @@ describe('on teardown', () => {
};

it('does not start the outgoing request on immediate teardowns', () => {
fetch.mockRejectedValue(abortError);
fetch.mockResolvedValue(new Response('text', { status: 200 }));

const { unsubscribe } = pipe(
makeFetchSource(queryOperation, 'https://test.com/graphql', {}),
Expand All @@ -174,20 +174,22 @@ describe('on teardown', () => {

unsubscribe();
expect(fetch).toHaveBeenCalledTimes(0);
expect(abort).toHaveBeenCalledTimes(1);
expect(abort).toHaveBeenCalledTimes(0);
});

it('aborts the outgoing request', async () => {
fetch.mockRejectedValue(abortError);
fetch.mockResolvedValue(new Response('text', { status: 200 }));

const { unsubscribe } = pipe(
makeFetchSource(queryOperation, 'https://test.com/graphql', {}),
subscribe(fail)
);

await Promise.resolve();

unsubscribe();

// NOTE: We can only observe the async iterator's final run after a macro tick
await new Promise(resolve => setTimeout(resolve));
expect(fetch).toHaveBeenCalledTimes(1);
expect(abort).toHaveBeenCalledTimes(1);
});
Expand Down
Loading