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

[polyfills] Bundling of package dependencies #237

Merged
merged 31 commits into from
Jul 29, 2024
Merged

[polyfills] Bundling of package dependencies #237

merged 31 commits into from
Jul 29, 2024

Conversation

Chriztiaan
Copy link
Contributor

@Chriztiaan Chriztiaan commented Jul 22, 2024

Introduction

This PR aims to reduce the amount of setup required to use the PowerSync-js SDKs. This setup typically includes dependencies/polyfills and sometimes minor code additions in a project to ensure that the polyfills are applied.

Original React Native Polyfills can be found here.

Original Web Polyfills can be found here. Web only needed buffer.

Rollup

By using Rollup for bundling we could include and process the polyfills so that consumers of the common, web, and react-native packages no longer need the majority of the listed polyfills.
This was done using a combination rollup plugins:

  • The nodeResolve plugin, which helps with including code from the dependencies in the bundle.
  • The inject plugin, which allows you to inject code on global references (instead of simply re-assigning global references).
  • Declaring some dependencies as external as to not include them in the bundle explicitly.

This bundling was only applied to common and react-native, as there was a few blockers found with web's workers and no additional setup gains needed beyond what web inherits by being a consumer of common.

Vendoring Crypto

For react native BSON has a crypto warning which I silenced by vendoring/modifying the react-native-get-random-values package so that it doesn't act as a polyfill but rather a ponyfill. The ponyfill is then injected global crypto references.

Checklist

  • Updated Readmes
  • Tested on an end user's setup
  • Updated demo projects to reflect changes
  • Updated gitbook docs (will wait on this)
  • Tested

Testing

To verify that this diff works I ran manual tests against the majority of the demos + diag tool inside the monorepo as well as against standalone copies. Before the tests I removed all polyfill related code/dependencies that are no longer needed.
For the react-native-supabase-todolist and react-supabase-todolist demos I tested both HTTP and Web socket connectionmethods.

Bundle sizes

Since dependencies are being bundled in our packages now it's worth looking at the changes in the packages:

Original:
Common: 250 KB
Web: 428 KB
React Native: 30 KB

After:
Common: 4.5 MB (1.4Mb for JS, 3.2MB for type map)
Web: 345KB
React Native: 5.8 MB (1.7Mb for js, 4.2Mb for type map)

With Terser:
Common: 3.1 MB (830Kb for js, 2.3 MB for typings)
Web: 330KB
React Native: 4.2 MB (1.3 MB for js, 3 MB for typings)

Effective change if using terser (minified) (no sourcemap in release):
Common: 250 KB -> 830KB
Web: 428 KB -> 214 KB
React Native: 30KB-> 1.3 MB

It looks like a lot of the bloat is coming from having bundled text-encoding twice (once from BSON that is prebundling it, and once from our side).

However, after comparing different react-native demo builds - it doesn't appear to make a substantial difference in a final app build (100KB Variance).

Future work

  • Consider removing the polyfill for @azure/core-asynciterator-polyfill. I didn't do this as it's supported an optional way of using the SDKs, and might have a few unknowns alongside the babel setup.
  • Bundling on web, which would have to be done with Vite instead of Rollup due to worker issues.
  • Currently the Rollup configs take code already transpiled by TSC. Can consider adding a typescript step to the plugins list.
  • Consider polyfills that are either faster or smaller like text-encoding vs fast-text-encoding (650kB vs 32kB). Initial investigation doesn't prove to work after trying to vendor FTE, which is only really impacting the HTTP method - maybe a suggestion would be to get users over to websockets instead assuming the performance is better. (This is only problematic now since users can't apply their own polyfills after this PR is merged). Benchmarks show that it's substantially faster.

stevensJourney and others added 27 commits June 3, 2024 09:59
# Conflicts:
#	packages/react-native/package.json
#	pnpm-lock.yaml
# Conflicts:
#	packages/react-native/package.json
#	pnpm-lock.yaml
# Conflicts:
#	packages/common/package.json
#	packages/react-native/package.json
#	packages/web/package.json
#	pnpm-lock.yaml
# Conflicts:
#	packages/react-native/package.json
#	pnpm-lock.yaml
Copy link

changeset-bot bot commented Jul 22, 2024

🦋 Changeset detected

Latest commit: 5642b97

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@powersync/attachments Minor
@powersync/kysely-driver Minor
@powersync/react Minor
@powersync/vue Minor
@powersync/web Minor
@powersync/react-native Minor
@powersync/common Minor
@powersync/diagnostics-app Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

'@powersync/web > lodash/throttle',
'@powersync/web > can-ndjson-stream'
]
include: []
Copy link
Contributor Author

@Chriztiaan Chriztiaan Jul 22, 2024

Choose a reason for hiding this comment

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

By having an empty array instead of omitting the field, it silences a vite warning.

@Chriztiaan Chriztiaan changed the title Bundling of package dependencies - Reduce Polyfills [polyfills] Bundling of package dependencies Jul 22, 2024
@Chriztiaan Chriztiaan marked this pull request as ready for review July 22, 2024 14:01
stevensJourney
stevensJourney previously approved these changes Jul 24, 2024
Copy link
Collaborator

@stevensJourney stevensJourney left a comment

Choose a reason for hiding this comment

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

Happy with this from my side

@Chriztiaan Chriztiaan requested a review from rkistner July 24, 2024 07:41
rkistner
rkistner previously approved these changes Jul 24, 2024
# Conflicts:
#	packages/react-native/package.json
#	pnpm-lock.yaml
@Chriztiaan Chriztiaan dismissed stale reviews from rkistner and stevensJourney via 5642b97 July 29, 2024 07:11
@Chriztiaan Chriztiaan merged commit 02ae5de into main Jul 29, 2024
4 checks passed
@Chriztiaan Chriztiaan deleted the prebundling branch July 29, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants