-
Notifications
You must be signed in to change notification settings - Fork 576
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
Make BSON an optional dependency #6017
Conversation
* Ensures that we don't require bson is installed to use `realm` v12
"debug": "^4.3.4", | ||
"node-fetch": "^2.6.9", | ||
"node-machine-id": "^1.1.12", | ||
"prebuild-install": "^7.1.1" | ||
}, | ||
"optionalDependencies": { |
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.
I don't think this is the intended use of optionalDependencies
as per this description:
If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies object.
I don't see a valid case where the installation of the bson
package would fail, as this happens mostly if it fails to download or build a binary for an unsupported target OS or similar failure during a postinstall
scripts execution.
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.
What you might be looking for is the peerDependenciesMeta
which can mark a peer dependency as optional, but I don't think that's what we want, since the internals of our SDK relies on it being installed and available at runtime.
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.
I've been experimenting with this, and have also found that the peerDependency
is the correct way of going about this. I was under the assumption that npm
wouldn't install bson
on npm install
, but it does.
I think in it's current form on main
the package.json is fine. We should stay on v4 for now, as the react-native tests fail using v5
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.
Yeah - a missing peer dependency is automatically installed since NPM v7.
What issue is this causing? A link would be great 👍
The peer dependency is declaring a dependency on |
After trying out |
What, How & Why?
bson
dependencybson
was a dependency ofrealm
and not apeerDependency
.bson
was added as apeerDependency
, causing the issue.This PR reverts the changes to v12 regarding
bson
, but also addsbson
as an optional dependency, freezing it to a v4 if it's installed locally, since v5 is not directly compatible withreact-native
☑️ ToDos
Compatibility
label is updated or copied from previous entryCOMPATIBILITY.md
package.json
s (if updating internal packages)Breaking
label has been applied or is not necessaryIf this PR adds or changes public API's: