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

fix: move mongodb to peerDependencies #435

Merged
merged 8 commits into from
Sep 17, 2021
Merged

Conversation

rfox12
Copy link
Contributor

@rfox12 rfox12 commented Aug 26, 2021

In order to allow connect-mongo to adapt to internal Typescript changes (that do not break the interface), we can specify mongodb as a peer dependency--allowing end users to use any version of mongodb they want as long as it's a minor release of driver version 4.1.x

Should close issue: #434 (comment) and issue #433 (comment) and #436 (comment)

In order to allow `connect-mongo` to adapt to internal Typescript changes (that do not break the interface), we can specify `mongodb` as a peer dependency--allowing end users to use any version of mongodb they want as long as it's a minor release of driver version 4.1.x
@mingchuno
Copy link
Collaborator

@rfox12 You package.json break. Could you help fix it, make sure test cases pass and also update CHANGELOG.md ?

@rfox12
Copy link
Contributor Author

rfox12 commented Sep 9, 2021

@mingchuno I finally got around to finishing out this PR. All 23 tests passed for both version 4.1.0 and 4.1.1 versions of the mongodb driver. So this PR should fix problems that people are having with Typescript types being incompatible (regardless of whether they are on 4.1.0 or 4.1.1). And hopefully this means we don't have to worry about the changes when 4.1.2 (or whatever) is released.

The way you test a peer dependency is to also include the peer version you want to test in your devDependencies section of the package.json. So in this case, I tested both 4.1.0 and 4.1.1 of mongodb by making that change and building / testing twice.

Please take a look, I think this is ready for release?

@lmeerma
Copy link

lmeerma commented Sep 15, 2021

Any update when this will be merged?

@rfox12
Copy link
Contributor Author

rfox12 commented Sep 16, 2021

@jdesboeufs @mingchuno My organization is happy to be a maintainer of connect-mongo. We rely on it.

Copy link
Collaborator

@mingchuno mingchuno left a comment

Choose a reason for hiding this comment

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

PR LGTM

@mingchuno mingchuno changed the title Move mongodb to peerDependencies fix: move mongodb to peerDependencies Sep 17, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2021

Codecov Report

Merging #435 (6e3458a) into master (3e27376) will decrease coverage by 0.55%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #435      +/-   ##
==========================================
- Coverage   82.32%   81.76%   -0.56%     
==========================================
  Files           2        2              
  Lines         181      181              
  Branches       42       42              
==========================================
- Hits          149      148       -1     
  Misses         22       22              
- Partials       10       11       +1     
Impacted Files Coverage Δ
src/lib/MongoStore.ts 81.56% <0.00%> (-0.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e27376...6e3458a. Read the comment docs.

@mingchuno
Copy link
Collaborator

@rfox12 Well, I am also a random guy out there that are using this repo and found it is unmaintained. So I ask @jdesboeufs if I can pick it up. I will tried to be here more often. Sadly, I don't have the admin access of this repo too.

@mingchuno mingchuno merged commit 2a2cd78 into jdesboeufs:master Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants