-
Notifications
You must be signed in to change notification settings - Fork 351
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
Bump neo4j-driver to latest version, and update all requisite dependencies #1983
Bump neo4j-driver to latest version, and update all requisite dependencies #1983
Conversation
"express": "^4.15.4", | ||
"file-loader": "^2.0.0", | ||
"fork-ts-checker-notifier-webpack-plugin": "^3.0.0", | ||
"fork-ts-checker-webpack-plugin": "^5.0.14", | ||
"html-loader": "^0.5.5", | ||
"html-webpack-plugin": "^4.5.2", | ||
"husky": "^0.14.3", | ||
"jest": "^25.2.4", | ||
"jest": "^26.6.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v25 has a peer dependency of typescript <v4, so we best update the version
however >v27, we run into a host of problems with jestdom and timers that I was unable to resolve
after v27, the jestdom environment is no longer included in the main jest package, and must be imported separately
as soon as this happens, many tests start throwing errors that setImmediate
is undefined
there's a lengthy thread here prisma/prisma#8558 with several workarounds
however, of the solutions I've tried, I did not get a successful test run
global.setImmediate = jest.useRealTimers
caused tests to hang until they timed out
import { setImmediate } from 'timers;
global.setImmediate = setImmediate
caused any test that relied on RXJS to throw an error that bind
was undefined
we also started getting snapshot test failures.. the updates to the snapshots were fairly trivial.. removing Object
and Array
annotations.. however it was beginning to increase the surface of the change
sticking to v26 negates these issues, although I think we should revisit them soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we'd want to invest in the build setup for browser I think it'd make sense to go to vite & vitest, but I don't think it's worth the time
"ts-loader": "^8.0.2", | ||
"typescript": "^3.9.5", | ||
"typescript": "^4.6.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
introducing the latest driver version also introduced some type errors at compile time
here, I've set the version of typescript to match that of the neo4j js driver
@@ -117,22 +117,22 @@ | |||
"cross-env": "^6.0.3", | |||
"css-loader": "^1.0.0", | |||
"cypress": "^8.3.1", | |||
"eslint": "^7.7.0", | |||
"eslint": "^8.57.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the version of eslint we were using similarly had a peer dependency on typescript <v4
v9 has some breaking changes with eslint config, again, something we should address, but to keep the change surface low, I've gone with v8
@@ -72,7 +72,7 @@ export type CypherBasicPropertyType = | |||
| number | |||
| string | |||
| Integer | |||
| BigInt | |||
| bigint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eslint --fix
introduced this change
@@ -40,6 +40,6 @@ export function mapObjectValues<A, B>( | |||
) | |||
} | |||
|
|||
export function keys<T>(object: T): Array<keyof T> { | |||
export function keys<T extends {}>(object: T): Array<keyof T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixes a compile error where Object.keys(object)
doesn't like the generic argument T as is
having T extend object satisfies the param types
Overload 1 of 2, '(o: {}): string[]', gave the following error.
Argument of type 'T' is not assignable to parameter of type '{}'.
Overload 2 of 2, '(o: object): string[]', gave the following error.
Argument of type 'T' is not assignable to parameter of type 'object'.
.eslintrc.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following the eslint upgrade, the following rules started piping up - these additions add around 300 new warnings, for a total of ~3900
src/shared/utils/typeguards.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding these typeguards to satisfy the Error type requirements that began to error as a result of the typescript upgrade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work with all the version fiddling ⭐
"express": "^4.15.4", | ||
"file-loader": "^2.0.0", | ||
"fork-ts-checker-notifier-webpack-plugin": "^3.0.0", | ||
"fork-ts-checker-webpack-plugin": "^5.0.14", | ||
"html-loader": "^0.5.5", | ||
"html-webpack-plugin": "^4.5.2", | ||
"husky": "^0.14.3", | ||
"jest": "^25.2.4", | ||
"jest": "^26.6.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we'd want to invest in the build setup for browser I think it'd make sense to go to vite & vitest, but I don't think it's worth the time
This PR bumps the version of the neo4j javascript driver to the latest version, 5.26.0.
I am also updating a number of requisite dependencies to accommodate the driver version bump, these I have annotated separately.