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.

Sign up for GitHub

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: support Node.js 18 #283

Closed
wants to merge 5 commits into from
Closed

feat: support Node.js 18 #283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
419cee7
build(deps-dev): upgrade to node 18 and jest 28, add node 18 to test …
milesrichardson Aug 29, 2022
95c0d16
fix(fetch): correctly intercept requests with `URL` inputs
milesrichardson Aug 29, 2022
f0c8438
fix(ipv6): serialize IPv6 hosts correctly in `normalizeClientRequestA…
milesrichardson Aug 29, 2022
737d6b0
build(deps-dev): upgrade `page-with` to fix IPv6 compatibility in Nod…
milesrichardson Sep 28, 2022
75c55d3
build(deps-dev): upgrade `@open-draft/test-server` to fix IPv6 compat…
milesrichardson Sep 28, 2022
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
node: [14, 16]
node: [14, 16, 18]
steps:
- name: Checkout
uses: actions/checkout@v2
Expand Down
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
v16.14.0
v18.8.0
13 changes: 7 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@
"devDependencies": {
"@commitlint/cli": "^16.0.2",
"@commitlint/config-conventional": "^16.0.0",
"@open-draft/test-server": "^0.4.2",
"@open-draft/test-server": "^0.5.0",
"@ossjs/release": "^0.3.0",
"@types/cors": "^2.8.12",
"@types/express": "^4.17.13",
"@types/express-rate-limit": "^6.0.0",
"@types/follow-redirects": "^1.14.1",
"@types/jest": "^27.0.3",
"@types/jest": "^28.1.8",
"@types/node": "^16.11.26",
"@types/node-fetch": "2.5.12",
"@types/supertest": "^2.0.11",
Expand All @@ -54,14 +54,15 @@
"express-rate-limit": "^6.3.0",
"follow-redirects": "^1.15.1",
"got": "^11.8.3",
"jest": "^27.4.3",
"jest": "^28.1.3",
"jest-environment-jsdom": "^28.1.3",
"node-fetch": "2.6.7",
"page-with": "^0.5.1",
"page-with": "^0.6.0",
"rimraf": "^3.0.2",
"simple-git-hooks": "^2.7.0",
"superagent": "^6.1.0",
"supertest": "^6.1.6",
"ts-jest": "^27.1.1",
"ts-jest": "^28.0.8",
"typescript": "4.3.5",
"wait-for-expect": "^3.0.2"
},
Expand Down Expand Up @@ -93,4 +94,4 @@
"path": "./node_modules/cz-conventional-changelog"
}
}
}
}
2 changes: 1 addition & 1 deletion src/interceptors/ClientRequest/NodeClientRequest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ test('emits the ECONNREFUSED error connecting to an inactive server given no moc
request.on('error', (error: ErrorConnectionRefused) => {
expect(error.code).toEqual('ECONNREFUSED')
expect(error.syscall).toEqual('connect')
expect(error.address).toEqual('127.0.0.1')
expect(['127.0.0.1', '::1']).toContain(error.address)
expect(error.port).toEqual(12345)

done()
Expand Down
8 changes: 7 additions & 1 deletion src/interceptors/fetch/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,13 @@ export class FetchInterceptor extends Interceptor<HttpRequestEventMap> {
globalThis.fetch = async (input, init) => {
const request = new Request(input, init)

const url = typeof input === 'string' ? input : input.url
const url =
typeof input === 'string'
? input
: input instanceof URL
? input.href
: input.url

Comment on lines -43 to +49
Copy link
Author

@milesrichardson milesrichardson Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the only user-facing change, to select .href in the case that input is an instance of URL.

I don't fully grok the root cause of this - update to JSDom maybe? But I note that it fixed a type error, which was reported in my VSCode with latest TypeScript but not by any build script. It also fixed some tests, but I can't remember exactly which, since this was one of multiple bugs fixed simultaneously.

It looks like a safe change to me, though, since the new branch should only evaluate when input instanceof URL, which (AFAICT) previously could only result in an error since there is no valid url property on an instance of URL. And all tests still pass in Node v14/v16, although that's not necessarily unexpected if the root cause was an update to jest-environment-jsdom.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this change for type compliance only? Afaik Fetch supports URL as input. Is that no longer the case for Node.js fetch?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I'm looking more closely at this now. Reverting this change does not cause any tests to fail on my local (non-IPv6) system. I also checked in GitHub Actions, and no tests failed there either. So we could revert it, but let me first check if it breaks any tests in the next branch following this one. I think it could fix a real issue that was only exposed by a TypeScript error, but I don't have any regression test to prove that.

The reason it's a type error in VSCode but not any of the build scripts is that VSCode uses version 4.7.3 while this package uses version 4.3.5, and the type error is related to the latest version of the fetch API from lib.dom.d.ts. So the type error is likely indicating a real bug, even if there are no tests for it. Likely the bug would surface in some environments (browser vs. Node) but not others.

Specifically, the error is:

Property 'url' does not exist on type 'Request | URL'.
  Property 'url' does not exist on type 'URL'.ts(2339)

as reported in VSCode:

image

Note that it's inferring the type of input as RequestInfo which it infers from the definition of globalThis.fetch in the latest TypeScript lib.dom.d.ts:

declare function fetch(input: RequestInfo | URL, init?: RequestInit): Promise<Response>;

visually:

image

Whether or not this indicates a bug at runtime depends on the fetch API provided by the environment. If we're using a subset of that API, then we should use module augmentation to keep the TypeScript API consistent with what we expect at runtime (although admittedly it won't be an issue until we upgrade the version of TypeScript used in the build scripts).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so reverting this change does have an effect on a downstream dependency - namely, the msw branch where I'm adding support for node-native interception. It causes tests to fail on IPv6 systems that were otherwise already fixed with this commit.

So, from a backwards compatibility perspective, it would be safer to revert this change. But from a forward compatibility perspective, we need it if we want to operate properly on RequestInfo objects in Node 18. The balance seems to weigh in favor of keeping the commit.

If you're okay with keeping this, then the PR is ready to merge

const method = request.method

this.log('[%s] %s', method, url)
Expand Down
13 changes: 12 additions & 1 deletion src/utils/getUrlByRequestOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ function getAuthByRequestOptions(options: ResolvedRequestOptions) {
}
}

/**
* Returns true if host looks like an IPv6 address without surrounding brackets
* It assumes any host containing `:` is definitely not IPv4 and probably IPv6,
* but note that this could include invalid IPv6 addresses as well.
*/
function isRawIPv6Address(host: string) {
return host.includes(':') && !host.startsWith('[') && !host.endsWith(']')
}

/**
* Creates a `URL` instance from a given `RequestOptions` object.
*/
Expand All @@ -85,7 +94,9 @@ export function getUrlByRequestOptions(options: ResolvedRequestOptions): URL {
debug('port', port)
debug('path', path)

const baseUrl = `${protocol}//${host}`
// NOTE: as of node >= 17, hosts (including "localhost") can resolve to IPv6
// addresses, so construct valid URL by surrounding IPv6 host with brackets
const baseUrl = `${protocol}//${isRawIPv6Address(host) ? `[${host}]` : host}`
debug('base URL:', baseUrl)

const url = options.uri ? new URL(options.uri.href) : new URL(path, baseUrl)
Expand Down
2 changes: 1 addition & 1 deletion test/jest.expect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { AsymmetricMatcher } from 'expect'
import { HeadersObject } from 'headers-polyfill'
import { AsymmetricMatcher } from 'expect/build/asymmetricMatchers'

/**
* A custom asymetric matcher that asserts the `Headers` object.
Expand Down
Loading