Skip to content

Commit

Permalink
Run tests with all Node frameworks
Browse files Browse the repository at this point in the history
  • Loading branch information
ardatan committed Dec 27, 2024
1 parent 3419cba commit 69a2b8e
Show file tree
Hide file tree
Showing 18 changed files with 1,125 additions and 423 deletions.
54 changes: 54 additions & 0 deletions .changeset/warm-plums-grin.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
---
'@whatwg-node/node-fetch': patch
---

- Normalization fixes on `URL`

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

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

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'
```
1 change: 1 addition & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ on:

env:
NODE_OPTIONS: --max-old-space-size=4096
NODE_NO_WARNINGS: 1
CI: true

jobs:
Expand Down
1 change: 1 addition & 0 deletions .yarnrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ignore-engines true
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"@theguild/prettier-config": "3.0.0",
"@types/jest": "29.5.14",
"@types/node": "22.10.2",
"@types/react": "19.0.2",
"@types/react-dom": "19.0.2",
"@typescript-eslint/eslint-plugin": "8.18.2",
"@typescript-eslint/parser": "8.18.2",
Expand Down
17 changes: 16 additions & 1 deletion packages/node-fetch/src/ReadableStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,22 @@ export class PonyfillReadableStream<T> implements ReadableStream<T> {
}

[Symbol.asyncIterator]() {
return this.readable[Symbol.asyncIterator]();
const iterator = this.readable[Symbol.asyncIterator]();
return {
next: () => iterator.next(),
return: () => {
if (!this.readable.destroyed) {
this.readable.destroy();
}
return iterator.return?.() || fakePromise({ done: true, value: undefined });
},
throw: (err: Error) => {
if (!this.readable.destroyed) {
this.readable.destroy(err);
}
return iterator.throw?.(err) || fakePromise({ done: true, value: undefined });
},
};
}

tee(): [ReadableStream<T>, ReadableStream<T>] {
Expand Down
13 changes: 11 additions & 2 deletions packages/node-fetch/src/URL.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,22 @@ export class PonyfillURL extends FastUrl implements URL {
return;
}
this.parse(url, false);
// `fast-url-parser` cannot handle ipv6 hosts correctly
// `fast-url-parser` incorrectly removes `[IPV6]` in the hostname
// Then `hostname` doesn't include `[` and `]` for IPv6 URLs
// This breaks implementations that rely on `hostname` to include `[` and `]`
if (
(url.startsWith('http://[') || url.startsWith('https://[')) &&
IPV6_REGEX.test(this.hostname)
) {
this.hostname = `[${this.hostname}]`;
}
// `fast-url-parser` incorrectly handle URLs with ports without pathnames correctly like
// `http://localhost:8080?foo=bar` -> `http://localhost/:8080?foo=bar`
if (url.includes(`${this.hostname}:`) && !this.port && this.pathname.startsWith('/:')) {
this.port = this.pathname.slice(2);
this.pathname = '/';
this.host = `${this.hostname}:${this.port}`;
}
if (base) {
const baseParsed = typeof base === 'string' ? new PonyfillURL(base) : base;
this.protocol ||= baseParsed.protocol;
Expand All @@ -36,7 +45,7 @@ export class PonyfillURL extends FastUrl implements URL {
}

get origin(): string {
return `${this.protocol}//${this.host}${this.port ? `:${this.port}` : ''}`;
return `${this.protocol}//${this.hostname}${this.port ? `:${this.port}` : ''}`;
}

private _searchParams?: PonyfillURLSearchParams;
Expand Down
38 changes: 38 additions & 0 deletions packages/node-fetch/tests/URL.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,36 @@
import { PonyfillURL } from '../src/URL.js';

describe('URL', () => {
const urlExamples = [
'https://example.com/?foo=bar&bar=qux',
'http://[::1]:8080',
'http://localhost:8080?foo=bar',
];
for (const urlExample of urlExamples) {
describe(urlExample, () => {
it('should parse and stringify URLs', () => {
const url = new PonyfillURL(urlExample);
expect(url.toString()).toBe(urlExample.replace('8080', '8080/'));
});
it('should parse the URLs as expected', () => {
const ponyfillUrl = new PonyfillURL(urlExample);
const nativeUrl = new URL(urlExample);
expect(ponyfillUrl.toString() || null).toBe(nativeUrl.toString() || null);
expect(ponyfillUrl.protocol || null).toBe(nativeUrl.protocol || null);
expect(ponyfillUrl.host || null).toBe(nativeUrl.host || null);
expect(ponyfillUrl.hostname || null).toBe(nativeUrl.hostname || null);
expect(ponyfillUrl.port || null).toBe(nativeUrl.port || null);
expect(ponyfillUrl.pathname || null).toBe(nativeUrl.pathname || null);
expect(ponyfillUrl.search || null).toBe(nativeUrl.search || null);
ponyfillUrl.searchParams.forEach((value, key) => {
expect(nativeUrl.searchParams.get(key) || null).toBe(value || null);
});
expect(ponyfillUrl.username || null).toBe(nativeUrl.username || null);
expect(ponyfillUrl.password || null).toBe(nativeUrl.password || null);
expect(ponyfillUrl.origin || null).toBe(nativeUrl.origin || null);
});
});
}
it('should parse search params', () => {
const url = new PonyfillURL('https://example.com/?foo=bar&foo=baz&bar=qux');
expect(url.searchParams.get('foo')).toBe('bar');
Expand All @@ -22,4 +52,12 @@ describe('URL', () => {
expect(url.hostname).toBe('[::1]');
expect(url.port).toBe('8000');
});
it('parses query strings with ports without pathname', () => {
const url = new PonyfillURL('http://localhost:8080?foo=bar');
expect(url.host).toBe('localhost:8080');
expect(url.hostname).toBe('localhost');
expect(url.pathname).toBe('/');
expect(url.port).toBe('8080');
expect(url.search).toBe('?foo=bar');
});
});
9 changes: 7 additions & 2 deletions packages/server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,15 @@ app.route({
url: '/mypath',
method: ['GET', 'POST', 'OPTIONS'],
handler: async (req, reply) => {
const response = await myServerAdapter.handleNodeRequestAndResponse(req, reply, {
const response: Response = await myServerAdapter.handleNodeRequestAndResponse(req, reply, {
req,
reply
})

if (!response) {
return reply.status(404).send('Not Found')
}

response.headers.forEach((value, key) => {
reply.header(key, value)
})
Expand Down Expand Up @@ -200,7 +205,7 @@ import myServerAdapter from './myServerAdapter'
const app = new Koa()

app.use(async ctx => {
const response = await myServerAdapter.handleNodeRequest(ctx.req)
const response = await myServerAdapter.handleNodeRequestAndResponse(ctx.request, ctx.res, ctx)

// Set status code
ctx.status = response.status
Expand Down
3 changes: 3 additions & 0 deletions packages/server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,13 @@
"tslib": "^2.6.3"
},
"devDependencies": {
"@hapi/hapi": "^21.3.12",
"@types/express": "5.0.0",
"@types/koa": "^2.15.0",
"@types/node": "22.10.2",
"express": "4.21.2",
"fastify": "5.2.0",
"koa": "^2.15.3",
"react": "19.0.0",
"react-dom": "19.0.0"
},
Expand Down
4 changes: 2 additions & 2 deletions packages/server/test/abort.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { runTestsForEachFetchImpl } from './test-fetch';
import { runTestsForEachServerImpl } from './test-server';

describe('Request Abort', () => {
runTestsForEachServerImpl((server, _) => {
runTestsForEachServerImpl(server => {
runTestsForEachFetchImpl((_, { fetchAPI, createServerAdapter }) => {
it('calls body.cancel on request abort', done => {
const adapter = createServerAdapter(
Expand All @@ -24,7 +24,7 @@ describe('Request Abort', () => {
setTimeout(() => {
abortCtrl.abort();
}, 300);
});
}, 1000);
});
});
});
83 changes: 0 additions & 83 deletions packages/server/test/express.spec.ts

This file was deleted.

Loading

0 comments on commit 69a2b8e

Please sign in to comment.