-
Notifications
You must be signed in to change notification settings - Fork 1.8k
docs(NODE-2329): write upgrade and migration guide #2890
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
Conversation
f39b752 to
ebbd33a
Compare
emadum
left a comment
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 great, thank you for putting this together! 👏 Had a few small requests.
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
docs/CHANGES_4.0.0.md
Outdated
| - Use command monitoring: `client.on('commandStarted', (ev) => {})` | ||
| - Top-Level export no longer a function: `typeof require('mongodb') !== 'function'` | ||
| - Must construct a MongoClient and call `.connect()` on it. | ||
| - No more `Symbol` export, now `BSONSymbol` which is a deprecated BSON type |
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.
do we want to clarify the implications of BSONSymbol being a deprecated type?
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.
Wrote some text, looks good?
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.
maybe change the order and make one sentence?
"Existing BSON symbols in your database will be deserialized to a BSONSymbol instance; however, users should use plain strings instead of BSONSymbol"
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.
SGTM, done!
Co-authored-by: Daria Pardue <[email protected]>
Co-authored-by: Daria Pardue <[email protected]>
Co-authored-by: Daria Pardue <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
dariakp
left a comment
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.
Resolved all comments, just the BSONSymbol wording still an open question
docs/CHANGES_4.0.0.md
Outdated
| - Use command monitoring: `client.on('commandStarted', (ev) => {})` | ||
| - Top-Level export no longer a function: `typeof require('mongodb') !== 'function'` | ||
| - Must construct a MongoClient and call `.connect()` on it. | ||
| - No more `Symbol` export, now `BSONSymbol` which is a deprecated BSON type |
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.
maybe change the order and make one sentence?
"Existing BSON symbols in your database will be deserialized to a BSONSymbol instance; however, users should use plain strings instead of BSONSymbol"
SGTM, there is also now a page in the official driver docs describing the feature, we don't need to duplicate that info here. |
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
emadum
left a comment
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.
Probably the final round of feedback 🚀
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
Co-authored-by: Eric Adum <[email protected]>
emadum
left a comment
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.
LGTM 🎉
dariakp
left a comment
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.
LGTM
Co-authored-by: Eric Adum <[email protected]> Co-authored-by: Daria Pardue <[email protected]> Co-authored-by: Anna Henningsen <[email protected]>
Upgrade guide, moved the versioned api feature mention out of this file, I'm thinking we should break out features from changes, thoughts?