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!(NODE-4710): remove capital D ObjectID export #528

Merged
merged 4 commits into from
Dec 8, 2022

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Nov 23, 2022

Description

What is changing?

Goal: remove ObjectID capital letter "D" export

This PR prompted discoveries about our cross compat concerns. I've pulled the fixes out of this PR and we have a follow up NODE-4843

But just leaving the notes captured here (and they are copied to the follow up):

  • The EJSON test comparing against an old version of nodejs were not running, I've put in a patchwork solution (add copy to Uint8Array before called serialize) that we should improve in NODE-4843
  • The CI script that sets up npm does extra work that's no longer necessary and just adds useless logs to our test runs
  • We now install bson v1 as bson_legacy as a dev dependency for testing only, makes it easier than downloading it manually although we do already have a solution like that in another file, NODE-4843 will consolidate.

What is the motivation for this change?

Legacy export removal.

Double check the following

  • Ran npm run lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

}
newBsonObject.objectID.id.copy = function (buffer, offset, start, end) {
return buffer.set(this.subarray(start, end), offset)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, why wasn’t this necessary before? And doesn’t the fact that it requires monkey-patching basically mean that proper interop is gone anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that was a question I was going to bring up, IIRC we were concerned about the interop b/c of a mix of bson versions in compass? with the removal of buffer the serialize functions in bson 1.x can't work with the types from 4.x when run in a non-nodejs env.

This wasn't failing before because these tests would attempt to import legacy bson but fallback to the current one so they ran but didn't compare anything unless you manually installed legacy bson, I set it up to always be available, uncovering these issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, I was mixing this up with the interop tests in test/node/bson_older_versions_tests.js – those still pass, so maybe this isn't really all that problematic? It probably makes sense to adjust the usesBufferPolyfill logic there, in any case.

concerned about the interop b/c of a mix of bson versions in compass

It's not just Compass, it's about being able to use a 3.x or lower driver with bson 4.x or above. That's the situation Compass was in for a long time, but I assume it's not that uncommon for users in general to not use both latest driver and latest bson? Right now, this isn't an issue for us, since we're on the 4.x driver with 4.x bson.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well because the each driver depends directly on its BSON library version it should be tied together but understandably folks can add BSON as a separate dep and use types from there, which would break against an older driver. That's why the driver reexports BSON types but it doesn't reexport BSON APIs (serialize etc.) and maybe that's the short coming that would inspire folks to install bson themselves 🤔
I don't see how its possible that we make changes to 4.x that we don't also backport to 1.x to keep cross compat going, maybe that's something we have to consider, this is something we need clear messaging on regardless so I'll put this on our agenda for tech discussion.

Yea your set of tests that put all the bundles against each other etc still pass but only on nodejs and that's b/c we're still using Buffer when in that in env. And maybe it's okay for this incompatibility to exist in the "web" mode of the library. (Or maybe its worth patching 1.x to use .set 👀)

Copy link
Contributor

Choose a reason for hiding this comment

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

backport to 1.x to keep cross compat going, maybe that's something we have to consider

My gut feeling is that you don’t have to worry about cross compat support with 1.x and 5.x. You may have to worry about cross compat support with 4.x with 5.x, though?

And maybe it's okay for this incompatibility to exist in the "web" mode of the library.

I would say so, yeah. The driver is strictly a Node.js library, so the concern of using a driver that uses a different bson version than the one that’s being used directly doesn’t really apply to the web mode variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created https://jira.mongodb.org/browse/NODE-4843 we can add v4 as a new version to verify against in the matrix that "bson_older_versions_tests" checks.

@nbbeeken nbbeeken force-pushed the NODE-4710-rm-oid-capital branch 2 times, most recently from 5b3746f to 4ad5fbb Compare November 28, 2022 20:29
@nbbeeken nbbeeken marked this pull request as ready for review December 5, 2022 20:29
@durran durran added Primary Review In Review with primary reviewer, not yet ready for team's eyes Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Dec 7, 2022
@durran durran merged commit 8511225 into main Dec 8, 2022
@durran durran deleted the NODE-4710-rm-oid-capital branch December 8, 2022 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants