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-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

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Jan 5, 2023

Description

What is changing?

Add react-native exports specifier.
Set it to the commonjs bundle.
Document polyfills.

What is the motivation for this change?

To support react native.

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
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken changed the title fix(NODE-4927): exports in package.json for react native and document how to polyfill for BSON feat(NODE-4927): exports in package.json for react native and document how to polyfill for BSON Jan 5, 2023
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 5, 2023
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
nbbeeken and others added 3 commits January 5, 2023 13:55
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
@baileympearson baileympearson merged commit 3b4b61e into main Jan 5, 2023
@baileympearson baileympearson deleted the NODE-XXXX-support-rn branch January 5, 2023 20:34
"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

Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

I have concerns about the need for end-users to provide these polyfills.

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
3 participants