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

feat: update mongodb to major version 4 #7909

Closed
wants to merge 2 commits into from
Closed

feat: update mongodb to major version 4 #7909

wants to merge 2 commits into from

Conversation

klassm
Copy link
Contributor

@klassm klassm commented Jul 15, 2021

Mongo released a new major version of mongodb, which should (tm) be backwards compatible with older versions, but is essentially a whole rewrite of the library in typescript. You can find the release notes here.

The thing is: With npm dependencies and your own project upgrading to the new major version, typeorm will crash, as the API slightly changed. I guess typeorm should use the latest version, to here's the update to make it work again.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@klassm klassm changed the title feat: Update mongodb to major version 4 feat: update mongodb to major version 4 Jul 15, 2021
@imnotjames
Copy link
Contributor

imnotjames commented Jul 15, 2021

With this now any old version will crash, won't it?

Can you update the peer dependencies required versions to match?

We also now have another mismatch between ObjectID and ObjectId in a number of places.

@imnotjames
Copy link
Contributor

Also the write concern options were dropped, weren't they?

mongodb/node-mongodb-native#2642

The mongo typings we store are also now incorrect.

@imnotjames
Copy link
Contributor

Given how big of a change this is, would it be safer to pin the mongo node driver version to 3 for now?

@klassm
Copy link
Contributor Author

klassm commented Jul 15, 2021

Yes, but that means that everybody will need to pin it then :-( - that's also weird. I would still try to migrate the typings to mongo 4 (actually use the public typings from Mongo). But this involves public api changes then :-(

@imnotjames
Copy link
Contributor

Yes, but that means that everybody will need to pin it then :-(

Yes, it means they wouldn't be able to use node-mongo@4 with TypeORM until we figure out how we want to roll out these changes. It's been 2 days since they released v4, we can wait a little bit to upgrade. If we pin the peer dependency version then it will be clear to anyone that tries to use TypeORM with node-mongo that we do not yet support v4.

But this involves public api changes then :-(

Yeah, that's sort of the issue.

There's also cases where queries & usages of the Mongo TypeORM driver would break when upgrading from node-mongo 3 to 4 but keeping the same code.

I also feel like there's a few other deprecated features we were using but I cannot for the life of me remember at this point in time.

@klassm
Copy link
Contributor Author

klassm commented Jul 15, 2021

I already have 90% done, so I would just finish it today after lunch. There seems to be three methods that don't exist any more.
You can have a look and decide what to do :-)

@klassm
Copy link
Contributor Author

klassm commented Jul 15, 2021

There you go - it is now quite a big PR, but effectively it uses the native types from mongo now - so next time you will at least see directly that some types don't match anymore, e.g. because mongo updated.
I am curious what you will do now - just do the peer dependency for version 3, or rather the mongo version 4 upgrade. Problem is that the upgrade will not get easier over the time :-(

UpdateWriteOpResult
} from "./typings";
UpdateFilter, UpdateOptions, UpdateResult,
} from "mongodb";
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this wouldn't we have to define mongodb as a full dependency rather than a peer and dev dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No not really. At runtime the proper dependency is available, as it comes from the calling application. For compile time we have the one as dev dependency.
The only point is that we have to ensure that the peer dependency matches. But this also happened up to now (which is actually why I started investigating in typeorm).

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a good feeling this will fail during runtime even if we aren't using mongo for cases like the browser at the very least.

The only point is that we have to ensure that the peer dependency matches. But this also happened up to now (which is actually why I started investigating in typeorm).

There's a very specific way we do imports of these peer dependencies - using PlatformTools - so that we don't bring in the dependencies in cases where it would cause failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah now I now what you mean - it's because the import is evaluated when reading the files. But that would mean that we'd have to copy all the dependency types to typeorm again - that's also weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's also weird.

I 100% agree.

@imnotjames
Copy link
Contributor

I really, really, really want to get us to a place where we can split the drivers OUT from the rest of the package. Part of this is difficult because of how the MongoDB integration had been implemented. 😢

The big issue I have with this vs just pinning to node-mongo v3 is that it's a breaking change & I am not comfortable pushing for a major version bump of TypeORM for this. It's been 2 days since the node-mongo v4 release & it was an entire rewrite.

As much as I trust my friends over at mongo I'm very hesitant to make this jump. (That reminds me.. I should ask them for coffee & chat about this a bit.)

@klassm
Copy link
Contributor Author

klassm commented Jul 15, 2021

Yeah that's a good idea :-) But that means there is no real point in continuing here. On the upside of this, we found out what breaking changes really were there.

@ariesclark

This comment has been minimized.

@klassm
Copy link
Contributor Author

klassm commented Sep 12, 2021

@RubyBB We now decided to get rid of typeorm completely and use the mongo driver directly instead. Typeorm does not offer too much abstraction - so it simplifies the overall architecture.

@pleerock
Copy link
Member

pleerock commented Nov 9, 2021

Feel free to re-open in case of new interest.

@pleerock pleerock closed this Nov 9, 2021
@ariesclark
Copy link

Feel free to re-open in case of new interest.

This issue should say opened as there's no reason to not eventually update the dependencies (unless typeorm intends to remove mongodb support) and staying on old dependencies will only lead to problems.

@pleerock
Copy link
Member

This issue should say opened as there's no reason to not eventually update the dependencies (unless typeorm intends to remove mongodb support) and staying on old dependencies will only lead to problems.

Issue is closed because author showed that he doesn't have interest in this PR. If someone wants to make it on top of his changes, he should create a new PR.

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