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-4938): improve react native bundle experience #578

Merged
merged 6 commits into from Jun 7, 2023

Conversation

durran
Copy link
Member

@durran durran commented May 31, 2023

Description

Creates a new CJS bundle to be used with React Native.

What is changing?

  • Vendors the text-encoding and base64 modules.
  • Removes modifying the global object from the vendored modules.
  • Creates a new bson.rn.cjs artefact that imports the vendored functions for TextEncoder, TextDecoder, btoa and atob.
Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-4938

Release Highlight

Improved React Native experience

The BSON package now ships a bundle made to work on React Native without additional polyfills preconfigured. The necessary APIs (TextEncoder/TextDecoder & atob/btoa) are now vendored into the RN bundle directly. Users should still install react-native-get-random-values themselves to get securely generated UUIDs and ObjectIds. Read more in the React Native section of our readme.

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

baileympearson
baileympearson previously approved these changes May 31, 2023
@baileympearson baileympearson self-assigned this May 31, 2023
@baileympearson baileympearson added the Team Review Needs review from team label May 31, 2023
@nbbeeken nbbeeken changed the title feat(NODE-4938): vendor utf8 and base64 encoding feat(NODE-4938): improve react native bundle experience May 31, 2023
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

I updated the title to focus on the intended feature, feel free to fix it further. Let's also update the readme's section on RN with the new instructions and the intended support for this bundle

@durran
Copy link
Member Author

durran commented Jun 1, 2023

I updated the title to focus on the intended feature, feel free to fix it further. Let's also update the readme's section on RN with the new instructions and the intended support for this bundle

Readme updated.

@durran durran requested a review from nbbeeken June 1, 2023 15:44
nbbeeken
nbbeeken previously approved these changes Jun 1, 2023
kraenhansen
kraenhansen previously approved these changes Jun 7, 2023
README.md Outdated
@@ -188,23 +188,17 @@ 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:
BSON vendors the required polyfills for `TextEncoder`, `TextDecoder`, `atob`, `btoa` when using React Native. One
Copy link
Member

Choose a reason for hiding this comment

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

I suspect the term "vendors" might not be clear enough for publicly facing documentation. Perhaps it would make sense to include an explanation on what that means for the user.

Suggested change
BSON vendors the required polyfills for `TextEncoder`, `TextDecoder`, `atob`, `btoa` when using React Native. One
BSON vendors the required polyfills for `TextEncoder`, `TextDecoder`, `atob`, `btoa` imported from React Native and therefore doesn't expect users to polyfill these. One

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

README.md Outdated
@@ -188,23 +188,17 @@ 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:
BSON vendors the required polyfills for `TextEncoder`, `TextDecoder`, `atob`, `btoa` when using React Native. One
additional polyfill, `crypto.getRandomValues` is required and can be installed with the following command:
Copy link
Member

Choose a reason for hiding this comment

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

is required

Isn't this optional?

Suggested change
additional polyfill, `crypto.getRandomValues` is required and can be installed with the following command:
additional polyfill, `crypto.getRandomValues` is recommended and can be installed with the following command:

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@@ -0,0 +1,30 @@
import MagicString from 'magic-string';

const REQUIRE_TEXT_ENCODING =
Copy link
Member

Choose a reason for hiding this comment

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

This name seam to suggest only one of the polyfills are provided here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed name to REQUIRE_POLYFILLS.

@kraenhansen
Copy link
Member

kraenhansen commented Jun 7, 2023

An alternative I don't know if you've considered: If you had a separate input for the RN bundle (which would setup the polyfills and re-export src/index.ts), you could most likely avoid the code injection / RequireVendor all together. It would require that the runtime independent code stored the polyfilled APIs on an object that the wrapping input file would then inject its functions into before importing src/index.ts. We're already using this pattern in Realm JS.

If you had a separate input file for the RN bundle, it could make sense (not necessarily for this PR) to move this check and warning into the RN bundle to avoid relying on checks of the runtime (as that tends to be brittle in my experience).

BTW: I've built the PR and tested ObjectId and UUID in a React Native app 👍

@durran durran dismissed stale reviews from kraenhansen and nbbeeken via 667342b June 7, 2023 15:07
@durran
Copy link
Member Author

durran commented Jun 7, 2023

An alternative I don't know if you've considered: If you had a separate input for the RN bundle (which would setup the polyfills and re-export src/index.ts), you could most likely avoid the code injection / RequireVendor all together. It would require that the runtime independent code stored the polyfilled APIs on an object that the wrapping input file would then inject its functions into before importing src/index.ts. We're already using this pattern in Realm JS.

If you had a separate input file for the RN bundle, it could make sense (not necessarily for this PR) to move this check and warning into the RN bundle to avoid relying on checks of the runtime (as that tends to be brittle in my experience).

BTW: I've built the PR and tested ObjectId and UUID in a React Native app 👍

I think that may be something we can discuss and potentially ticket in the future.

@baileympearson baileympearson merged commit 7e16636 into main Jun 7, 2023
3 checks passed
@baileympearson baileympearson deleted the NODE-4938 branch June 7, 2023 17:23
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
4 participants