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.

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

chore(deps): update dependency mongoose to v7.6.4 #1825

Closed

Conversation

LiranBinshtok
Copy link

Which problem is this PR solving?

updating the mongoose version so higher version can be traced

Short description of the changes

@LiranBinshtok LiranBinshtok requested a review from a team November 25, 2023 13:51
Copy link

linux-foundation-easycla bot commented Nov 25, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

@david-luna david-luna left a comment

Choose a reason for hiding this comment

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

IMO this PR is adding support to a new major version v7 so I would update the range to >=5.9.7 <8.

@LiranBinshtok
Copy link
Author

@david-luna done. ty

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps interesting trivia about package-lock changes here: The change in mongodb@4 to mongodb@5 dep included this change in mongodb/package.json:

       "optionalDependencies": {
-        "@aws-sdk/credential-providers": "^3.186.0",
...
+      },
+      "peerDependencies": {
+        "@aws-sdk/credential-providers": "^3.188.0",
...
+      },
+      "peerDependenciesMeta": {
+        "@aws-sdk/credential-providers": {
+          "optional": true
+        },
...

That change resulted in npm deciding to not install the whole @aws-sdk/credential-providers optional dep tree.

@@ -52,7 +52,7 @@
"@types/node": "18.6.5",
"expect": "29.2.0",
"mocha": "7.2.0",
"mongoose": "6.11.5",
"mongoose": "^7.6.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The current latest is 7.6.6, so that could be bumped, but dependency automation will get it as well, so no worries.

@trentm
Copy link
Contributor

trentm commented Dec 1, 2023

Note that mongoose@7 bumps the min supported node to "node": ">=14.20.1" and this package currently has "node": ">=14.0". However, all instrumentations in this repo, except this one and the one for socket.io, have "node": ">=14". I've opened #1843 to move this package to use the latter.

In other words, IMO the bump to mongoose@7 is fine.

Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Ah, I approved too quickly. There are a number of type issues now with the bump to mongoose@7:

`npm run compile` error output
> @opentelemetry/[email protected] version:update
> node ../../../scripts/version-update.js

test/mongoose.test.ts:105:15 - error TS2559: Type '() => void' has no properties in common with type 'SaveOptions'.

105     user.save(() => {
                  ~~~~~~~

test/mongoose.test.ts:166:17 - error TS2339: Property 'remove' does not exist on type 'Document<unknown, {}, IUser> & IUser & { _id: ObjectId; }'.

166     await user!.remove();
                    ~~~~~~

test/mongoose.test.ts:176:13 - error TS2339: Property 'remove' does not exist on type 'Document<unknown, {}, IUser> & IUser & { _id: ObjectId; }'.

176       user!.remove({ overwrite: true }, () => {
                ~~~~~~

test/mongoose.test.ts:308:16 - error TS2339: Property 'update' does not exist on type 'Model<IUser, {}, {}, {}, Document<unknown, {}, IUser> & IUser & { _id: ObjectId; }, any>'.

308     await User.update(
                   ~~~~~~

test/mongoose.test.ts:535:70 - error TS2554: Expected 0-2 arguments, but got 3.

535       User.deleteOne({ email: '[email protected]' }, { lean: 1 }, () => {
                                                                         ~~~~~~~
536         const spans = getTestSpans();
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
... 
544         done();
    ~~~~~~~~~~~~~~~
545       });
    ~~~~~~~

test/mongoose.test.ts:571:17 - error TS2559: Type '(_err: any, createdUser: any) => void' has no properties in common with type 'SaveOptions'.

571       user.save((_err, createdUser) => {
                    ~~~~~~~~~~~~~~~~~~~~~~~~

test/mongoose.test.ts:571:18 - error TS7006: Parameter '_err' implicitly has an 'any' type.

571       user.save((_err, createdUser) => {
                     ~~~~

test/mongoose.test.ts:571:24 - error TS7006: Parameter 'createdUser' implicitly has an 'any' type.

571       user.save((_err, createdUser) => {
                           ~~~~~~~~~~~


Found 8 errors.

@trentm
Copy link
Contributor

trentm commented Dec 1, 2023

From https://github.com/Automattic/mongoose/releases/tag/7.0.0-rc0 and https://github.com/Automattic/mongoose/releases/tag/7.0.0 there are a number of breaking changes in mongoose 6 -> 7 that would need to be worked through. For example, changing from using callbacks to using promises. That will require some work on the tests to get them passing again.

@LiranBinshtok
Copy link
Author

From https://github.com/Automattic/mongoose/releases/tag/7.0.0-rc0 and https://github.com/Automattic/mongoose/releases/tag/7.0.0 there are a number of breaking changes in mongoose 6 -> 7 that would need to be worked through. For example, changing from using callbacks to using promises. That will require some work on the tests to get them passing again.

@trentm that will take me some time to fix but I will try to give it a go.

@LiranBinshtok
Copy link
Author

i will need to fix tests before reopening this. closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants