-
Notifications
You must be signed in to change notification settings - Fork 355
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
Update Mongo driver to v6.9.0
#4680
Conversation
- remove old @type definitions - mongo now provides typescript types
- fix date parsing - handle TS->JS for-loop type safety loss
✅ Deploy Preview for gallant-galileo-14878c canceled.
|
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.
Everything seems to be working great with the update! I left a few questions throughout.
@@ -116,10 +118,12 @@ export async function retrieveTodayCommentMetrics( | |||
continue; | |||
} | |||
|
|||
const mod = moderator; |
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.
just curious why we're assigning this here now? Looks like we're still using moderator
on line 124 though? (Same question about similar change below in the file too)
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.
it's a bug in typescript, it sometimes fails to process types after a continue
op. If you re-assign, it works. So I did that for the few lines that were "erroring" (but compile and run fine).
@@ -65,15 +70,16 @@ export default class Query<T> { | |||
* orderBy will apply sorting to the query filter when executed. | |||
* @param sort the sorting option for the documents | |||
*/ | |||
public orderBy(sort: object): Query<T> { | |||
public orderBy(sort: { [key: string]: SortDirection }): Query<T> { | |||
// todo: merge sort's together |
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.
Is this still a TODO?
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.
nope! I "sorted" this earlier! I will update!
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.
fixed!
// believe we don't need this since the new driver only uses | ||
// the new URL parser. But am leaving this here in case we have | ||
// issues with URL's and want to investigate into it. | ||
// useNewUrlParser: true, |
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.
👍
@@ -1,5 +1,5 @@ | |||
import { identity, isNumber } from "lodash"; | |||
import { MongoError } from "mongodb"; | |||
import { Collection, MongoError } from "mongodb"; | |||
import { v4 as uuid } from "uuid"; | |||
|
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.
I'm seeing a warning in the console: "(node:85676) [MONGODB DRIVER] Warning: cursor.count is deprecated and will be removed in the next major version, please use collection.estimatedDocumentCount
or collection.countDocuments
instead".
Looks like we are using deprecated count()
on line 135 in this file. Should we update to one of the suggested replacements now?
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.
looking into this, will see if I can update the query
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.
yup, was an easy fix, done!
Very nice! What mongo version are you looking to use with this? |
What does this PR do?
These changes will impact:
What changes to the GraphQL/Database Schema does this PR introduce?
None
Does this PR introduce any new environment variables or feature flags?
No
If any indexes were added, were they added to
INDEXES.md
?N/A
How do I test this PR?
This touches all of Coral's data retrieval and persistence, all features must be re-tested in a regression test.
Were any tests migrated to React Testing Library?
No
How do we deploy this PR?