Drop node 18 and 20. Add 24.#1708
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request drops support for Node.js 18 and 20, requiring Node.js 22 or greater. It updates the TypeScript compilation target to ES2022, adopts the node: prefix for built-in imports, and refactors Reader.open and WebServiceClient to use modern features like async/await, AbortSignal.timeout, and btoa. Feedback was provided regarding an unsafe type assertion (as Error) in the catch block of webServiceClient.ts, suggesting a safer check using instanceof Error to prevent potential runtime errors.
mm-kevcenteno
force-pushed
the
kevin/drop-old-node
branch
from
4f0cb32 to
54db174
Compare
mm-kevcenteno
force-pushed
the
kevin/drop-old-node
branch
from
f951e27 to
96f5fab
Compare
mm-kevcenteno
force-pushed
the
kevin/drop-old-node
branch
from
f1aab53 to
d44a77b
Compare
dhogan8
left a comment
dhogan8
left a comment
There was a problem hiding this comment.
I had a couple nits and a question.
| "jest": "^29.5.0", | ||
| "@types/jest": "^30.0.0", | ||
| "jest": "^30.0.0", | ||
| "ts-jest": "^29.1.0", |
There was a problem hiding this comment.
Do we also want to update this to 29.4.0 like the others?
There was a problem hiding this comment.
| "version": "7.0.0", |
To match the changelog
| error: `${error.name} - ${error.message}`, | ||
| url, | ||
| }); | ||
| const error = err instanceof DOMException ? err : new Error(String(err)); |
There was a problem hiding this comment.
| const error = err instanceof DOMException ? err : new Error(String(err)); | |
| const error = err instanceof Error || err instanceof DOMException ? err : new Error(String(err)); |
So that "Error - Error: Network Error" becomes "Error - Network Error" and we can avoid the duplication of Error.
| const expected = { | ||
| code: 'FETCH_ERROR', | ||
| error: `Error - ${error}`, | ||
| error: `Error - Error: ${error}`, |
There was a problem hiding this comment.
Should we revert this back per https://github.com/maxmind/GeoIP2-node/pull/1708/changes#r3357534039, or is there a reason to change it above?
3 participants
package-lock.json files were rebuilt because of missing transitive dependencies that caused
npm cito fail