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.

Sign up for GitHub

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-4927): exports in package.json for react native and document how to polyfill for BSON #550

Merged
merged 6 commits into from
Jan 5, 2023
Merged

feat(NODE-4927): exports in package.json for react native and document how to polyfill for BSON #550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
493cbe3
fix(NODE-XXXX): exports in package.json for react native and document…
nbbeeken Dec 28, 2022
e08bf78
Update global requirements text
nbbeeken Jan 5, 2023
5cefe2f
Update where to put polyfills
nbbeeken Jan 5, 2023
a84c658
nit fix text
nbbeeken Jan 5, 2023
c85cea4
Add clarity to the technical details of the react native import
nbbeeken Jan 5, 2023
dff0028
Update README.md
nbbeeken Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,40 @@ try {
}
```

## React Native

BSON requires that `TextEncoder`, `TextDecoder`, `atob`, `btoa`, and `crypto.getRandomValues` are available globally. These are present in most Javascript runtimes but require polyfilling in React Native. Polyfills for the missing functionality can be installed with the following command:
```sh
npm install --save react-native-get-random-values text-encoding-polyfill base-64
```

The following snippet should be placed at the top of the entrypoint (by default this is the root `index.js` file) for React Native projects using the BSON library. These lines must be placed for any code that imports `BSON`.

```typescript
// Required Polyfills For ReactNative
import {encode, decode} from 'base-64';
if (global.btoa == null) {
global.btoa = encode;
}
if (global.atob == null) {
global.atob = decode;
}
import 'text-encoding-polyfill';
Comment on lines +323 to +330
Copy link
Member

@kraenhansen kraenhansen Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It would be great if these could be included in the React Native bundle though.

I don't think it's too much to warn users and have them optionally import the react-native-get-random-values package for added security, but having these polyfills as strict requirement for React Native users (hence all Realm JS users) is a no-go in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a valid concern.

@nbbeeken would know better than I about the difficultly of bundling these the required dependencies in the react native build.

As an alternative, could realm provide these polyfills for users instead of 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.

Let's file a ticket for providing a polyfilled bundle for RN and we can fill it out with more detail on how the bundle should behave given that there is a mix of platforms and javascript engines that can be combined here.

Just adding background, one of our goals was to move away from bundling polyfills since they became part of the semantic version of this package despite being outside of our control. We became unable to pull in updates to the polyfills as they would break existing users but prevented other users from puling in the needed updates. Keeping polyfilling outside of a library's responsibility understandably adds friction but allows those that need the specific APIs working in a specific way to have control over that since not every polyfill is perfect for every use case.

Copy link
Member

@kraenhansen kraenhansen Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I understand that problem, we have similar issues in the Realm JS package. Ideally the bson package's source code should be runtime independent and therefore not rely on these globals in the first place. Instead it could have these functions "injected" by a platform specific index.ts wrapping the actual library. We're doing similar tricks on the upcoming refactor of the Realm JS SDK.

Technically the base-64 package could be a peer dependency and not included in the bundle, while it's still being imported by bson and injected into the "common" part of its code.

import 'react-native-get-random-values';
```

Finally, import the `BSON` library like so:

```typescript
import { BSON, EJSON } from 'bson';
```

This will cause React Native to import the `node_modules/bson/lib/bson.cjs` bundle (see the `"react-native"` setting we have in the `"exports"` section of our [package.json](./package.json).)

### Technical Note about React Native module import

The `"exports"` definition in our `package.json` will result in BSON's CommonJS bundle being imported in a React Native project instead of the ES module bundle. Importing the CommonJS bundle is necessary because BSON's ES module bundle of BSON uses top-level await, which is not supported syntax in [React Native's runtime hermes](https://hermesengine.dev/).

## FAQ

#### Why does `undefined` get converted to `null`?
Expand Down
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,11 @@
},
"main": "./lib/bson.cjs",
"module": "./lib/bson.mjs",
"browser": "./lib/bson.mjs",
"exports": {
"browser": "./lib/bson.mjs",
"import": "./lib/bson.mjs",
"require": "./lib/bson.cjs"
"require": "./lib/bson.cjs",
"react-native": "./lib/bson.cjs",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure on this, but I think the react-native property should be one level up, looking at the docs for the Metro config and the spec that it references.

Was this manually tested in a React Native project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I made a new project with react-native-cli and confirmed that this works with the bundling that comes with that setup, not sure if that is Metro. The "exports" property is standardized for nodejs, webpack, and rollup, I suspect it may just not be explicitly mentioned there?

We can get this into an alpha release and maybe you have projects with more diverse tooling worth testing to see if it gets pulled in correctly, I'll ping you on slack when its available

"browser": "./lib/bson.mjs"
},
"engines": {
"node": ">=14.20.1"
Expand Down