Skip to content

Commit

Permalink
F
Browse files Browse the repository at this point in the history
  • Loading branch information
ardatan committed Dec 27, 2024
1 parent f22f226 commit 763fdcd
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 14 deletions.
39 changes: 38 additions & 1 deletion .changeset/warm-plums-grin.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
'@whatwg-node/node-fetch': patch
---

- Normalization fixes on `URL`

Normalize the URL if it has a port and query string without a pathname;

```diff
Expand All @@ -14,4 +16,39 @@ Previously, it was normalized like below which was incorrect;
```diff
- http://example.com:80?query
+ http://example.com/:80?query
```
```

- Fix `URL.origin`

When the URL has a port, `origin` was doubling the port number;

```diff
- http://example.com:80:80
+ http://example.com:80
```

- Fix `ReadableStream[Symbol.asyncIterator]`

`ReadableStream` uses `Readable` so it uses `Symbol.asyncIterator` method of `Readable` but the returned iterator's `.return` method doesn't handle cancellation correctly. So we need to call `readable.destroy(optionalError)` manually to cancel the stream.

This allows `ReadableStream` to use implementations relying on `AsyncIterable.cancel` to handle cancellation like `Readable.from`

Previously the following was not handling cancellation;

```ts
const res = new ReadableStream({
start(controller) {
controller.enqueue('Hello');
controller.enqueue('World');
},
cancel(reason) {
console.log('cancelled', reason);
}
});

const readable = Readable.from(res);

readable.destroy(new Error('MY REASON'));

// Should log 'cancelled MY REASON'
```
4 changes: 2 additions & 2 deletions packages/node-fetch/src/ReadableStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,13 @@ export class PonyfillReadableStream<T> implements ReadableStream<T> {
if (!this.readable.destroyed) {
this.readable.destroy();
}
return iterator.return?.();
return iterator.return?.() || fakePromise({ done: true, value: undefined });
},
throw: (err: Error) => {
if (!this.readable.destroyed) {
this.readable.destroy(err);
}
return iterator.throw?.(err);
return iterator.throw?.(err) || fakePromise({ done: true, value: undefined });
},
};
}
Expand Down
11 changes: 6 additions & 5 deletions packages/server/test/node.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -383,10 +383,6 @@ describe('Node Specific Cases', () => {
const received: { status: number; statusText: string; resText?: string }[] = [];
for (const statusCodeStr in STATUS_CODES) {
const status = Number(statusCodeStr);
if (serverImplName === 'koa' && status === 425) {
// Koa gives 'Unordered Collection' for 425
continue;
}
if (status < 200) {
// Informational responses are not supported by fetch in this way
continue;
Expand Down Expand Up @@ -420,13 +416,18 @@ describe('Node Specific Cases', () => {
const res = await fetch(testServer.url, {
signal: ctrl.signal,
});
const expectedStatusText = STATUS_CODES[status];
let expectedStatusText = STATUS_CODES[status];
// Koa gives 'Unordered Collection' for 425
if (status === 425 && serverImplName === 'koa') {
expectedStatusText = 'Unordered Collection';
}
if (!expectedStatusText) {
throw new Error(`No status text for status ${status}`);
}
resText = await res.text();
expected.push({
status,
// Some servers like Koa has a different naming convention for status texts
statusText: expectedStatusText.toLowerCase(),
});
received.push({
Expand Down
8 changes: 2 additions & 6 deletions packages/server/test/request-listener.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,19 +106,15 @@ describe('Request Listener', () => {
const { 'content-length': contentLength1, ...toBeCheckedObj } = getHeadersObj(
toBeChecked.headers,
);
const { 'content-length': contentLength2, ...expectedObj } = getHeadersObj(
expected.headers,
);
const { 'content-length': contentLength2, ...expectedObj } = getHeadersObj(expected.headers);
expect(toBeCheckedObj).toMatchObject(expectedObj);
}

function compareResponse(toBeChecked: Response, expected: Response) {
const { 'content-length': contentLength1, ...toBeCheckedObj } = getHeadersObj(
toBeChecked.headers,
);
const { 'content-length': contentLength2, ...expectedObj } = getHeadersObj(
expected.headers,
);
const { 'content-length': contentLength2, ...expectedObj } = getHeadersObj(expected.headers);
expect(toBeCheckedObj).toMatchObject(expectedObj);
expect(toBeChecked.status).toBe(expected.status);
}
Expand Down

0 comments on commit 763fdcd

Please sign in to comment.