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

Add bson ^5 to peerDeps in addition to ^4 #5943

Closed
wants to merge 6 commits into from
Closed

Add bson ^5 to peerDeps in addition to ^4 #5943

wants to merge 6 commits into from

Conversation

elle-j
Copy link
Contributor

@elle-j elle-j commented Jun 27, 2023

What, How & Why?

NOTE: Wait to merge until a BSON release containing these changes is out. (See comment below)

Users of [email protected] would end up with the following error:

Unable to resolve "bson" from "node_modules/realm/dist/bundle.react-native.mjs"

If using bson v5+, the issue is resolved but with the following warning:

warning " > [email protected]" has incorrect peer dependency "bson@^4".

This PR adds the ^5 to the bson peer dependency.

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 🔀 Executed flexible sync tests locally if modifying flexible sync
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated

@kraenhansen
Copy link
Member

kraenhansen commented Jun 27, 2023

🤞 I can't completely recall why we needed to pin this to v4, I suspect I was concerned about the polyfills introduced by mongodb/js-bson#550 (which got shipped as part of v5.0.0). Also, for context a PR, which will vendor a couple of these polyfills has merged but is yet to be published, we might want to wait for the release of that and support only v4 || v5.4.0 (or whatever that version will be).

To ensure we're testing with bson@5 we should probably add a devDependency to the SDK package (in addition to the peer dependency) as well as the integration tests package .. and you might need to bump the version in realm-web to avoid conflicting versions (which would potentially prevent bson from being hoisted to the root). We've had issues with value comparisons before, where the test app would use one version of bson while the SDK used a different one and value instanceof bson.ObjectId would be incorrectly false.

packages/realm/package.json Outdated Show resolved Hide resolved
@kraenhansen
Copy link
Member

kraenhansen commented Jun 27, 2023

I'm surprised that users are seeing

Unable to resolve "bson" from "node_modules/realm/dist/bundle.react-native.mjs"

if they just allow NPM to install bson@v4 🤔 Perhaps it's related to that being a ESM, if so the change back to CJS might magically fix it.

@elle-j
Copy link
Contributor Author

elle-j commented Jun 27, 2023

@kraenhansen, thanks for some context PRs!

Regarding:

..we might want to wait for the release of that and support only v4 || v5.4.0..

If we'd wait, do you think it'd be okay to tell users to install v5+ but ignore the warning?

I'll make the bson ^5 a dev dependency in the SDK and tests, and do the change in realm-web as well.

@elle-j
Copy link
Contributor Author

elle-j commented Jun 30, 2023

Update: I've added to the PR description to wait with the merge until the related BSON changes are released.

@takameyer
Copy link
Contributor

I've updated the peerDependency to be ^4 || >=5.4.0. This should make sure we are always compatible and not have the polyfill issue

@takameyer
Copy link
Contributor

So one thing i've noted, is that if we have bson as a peerDependency than it will be required in the users project. I think this is not our intention, as this would break for most users as soon as they updated, since in older realm versions, bson was just a normal dependency. Therefore, I think we should change it to an optionalDependency to ensure that if a user is using their own version of bson, that it is within the compatible range for realm.

@takameyer
Copy link
Contributor

The React Native test are also failing for version 5.4.0. I believe there are missing polyfills. We may just have to revert to pinning this back to v4 and keeping it as an internal dependency.

Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

I think we need to review this further. Upgrading to v5 breaks the integration tests for React Native. I've created #6017 to ensure our users don't get errors thrown when installing realm regarding bson. This pins bson to v4 to ensure compatibility. We should look into this at another time #6018

@elle-j
Copy link
Contributor Author

elle-j commented Apr 11, 2024

Closing this (instead follow #6561).

@elle-j elle-j closed this Apr 11, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants