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: GZIP #109

Merged
merged 22 commits into from
Oct 23, 2024
Merged

feat: GZIP #109

merged 22 commits into from
Oct 23, 2024

Conversation

PuruVJ
Copy link
Contributor

@PuruVJ PuruVJ commented Oct 8, 2024

This adds withCompression method on DbConnection with the options gzip | none. We are dropping brotli here and erroring out anywhere where we recieve a brotli message back.

For GZIP, we are using the built-in DecompressionStream('gzip') constructor which is compatible with Chrome 80 onwards and NodeJS 17+. This is a standard parser run at the system level by the browser/platform rather than the heavy userland brotli.js, saving us from any inconsistency from userland solutions.

As soon as DecompressionStream('brotli') gains minimum viable support, we will add that as an option as well. Tracking this issue whatwg/compression#34

This brings down our bundle size from 72.1KB min+br to just 7.2KB! 🎉

Main change

DecompressionStream is a stream, aka it is asynchronous. This means we need to add a bunch of async and awaits wherever the decoding is going on. Is that going to affect us?

Copy link

changeset-bot bot commented Oct 8, 2024

🦋 Changeset detected

Latest commit: 0bf82ae

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

This PR includes changesets to release 3 packages
Name Type
@clockworklabs/spacetimedb-sdk Patch
@clockworklabs/test-app Patch
client-vite 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

Copy link

pkg-pr-new bot commented Oct 8, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@clockworklabs/spacetimedb-sdk@109

commit: 0bf82ae

@clockworklabs clockworklabs deleted a comment from pkg-pr-new bot Oct 8, 2024
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Minor questions but impl looks correct.

packages/test-app/src/App.tsx Outdated Show resolved Hide resolved
packages/test-app/src/App.tsx Outdated Show resolved Hide resolved
packages/test-app/src/main.tsx Show resolved Hide resolved
@gefjon
Copy link
Contributor

gefjon commented Oct 22, 2024

DecompressionStream is a stream, aka it is asynchronous. This means we need to add a bunch of async and awaits wherever the decoding is going on. Is that going to affect us?

This is difficult for me to answer given that we don't have any fully-integrated tests or working examples.

Does the user-facing API change? No more async/await points should be exposed to users now than were before, which should be zero (but who knows if it actually was).

Is it possible that messages will be processed out-of-order if an early message takes a very long time to decompress, while a later message takes a short time to decompress?

If the answer to both of these questions is "no," then this is fine. Otherwise, something somewhere needs to change.

@gefjon
Copy link
Contributor

gefjon commented Oct 22, 2024

Update: We determined that there is an out-of-order processing bug in this PR. The high-level solution will be to have a queue of handleOnMessage calls (or something like that), and zero-or-one handleOnMessage promises/futures. When one of those promises completes, grab the next call from the queue and start it. When the onMessage callback is invoked, check if a promise is already running; if it is, add the call to the queue rather than immediately spawning its promise.

@PuruVJ PuruVJ merged commit cf7b7d8 into main Oct 23, 2024
6 checks passed
@PuruVJ PuruVJ deleted the gzip branch October 23, 2024 16:57
@github-actions github-actions bot mentioned this pull request Oct 23, 2024
PuruVJ added a commit that referenced this pull request Oct 23, 2024
PuruVJ added a commit that referenced this pull request Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants