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

remove top level await #3427

Closed
wants to merge 1 commit into from
Closed

Conversation

natew
Copy link

@natew natew commented Dec 20, 2024

So this is horrible, and looking at the library this is a huge dependency we should probably try and remove right? Not sure the constraints here, but wanted to surface this.

The problem for us is top-level await isn't supported in RN.

Copy link

vercel bot commented Dec 20, 2024

@natew is attempting to deploy a commit to the Rocicorp Team on Vercel.

A member of the Team first needs to authorize it.

@aboodman
Copy link
Contributor

But can’t esbuild or whoever transform the code to not use top level await

@natew
Copy link
Author

natew commented Dec 20, 2024

We’re targeting commonjs and esbuild doesn’t support that combo. Don’t think there’s an easy way on our side other than something a bit weird.

What about a pure js implementation? Isn’t wasm rarely faster, plus the 30kb just for a hash is really blowing the budget if aiming for light weight

@tantaman
Copy link
Contributor

plus the 30kb just for a hash is really blowing the budget if aiming for light weight

30kb or 3kb?

CleanShot 2024-12-20 at 10 42 33

@tantaman
Copy link
Contributor

I think we'd still need to export a promise so callers can wait for xxhash to be ready.

@arv spent a whole lot of time on this problem so he should weigh in.

@tantaman
Copy link
Contributor

But I agree we should probably remove xxhash. It is only used to hash that AST at this point and nowhere else.

@natew
Copy link
Author

natew commented Dec 20, 2024

Oh sorry, I saw on npm page or something the 30kb stat but didn’t verify. Yea no strong opinions here beyond avoiding the top level await if possible. I can also fix it my side, long run I may need to in a more generic way.

@aboodman
Copy link
Contributor

aboodman commented Dec 20, 2024 via email

@arv
Copy link
Contributor

arv commented Dec 20, 2024

I would also like to remove the top level await.

There are a few reasons we kept it.

It is faster and smaller than all the pure js alternatives.

It is supported in all browsers since 2022!

I'm surprised you cannot configure expo to support modern JavaScript?

The reason why the top level one was hard to remove is that we hash the AST during the sync startup code and it would create an async requirement at startup which we really do not want to introduce.

@arv
Copy link
Contributor

arv commented Dec 20, 2024

A quick search shows that Hermes supports top level await. Maybe it just a matter of configuring things to not use cjs?

@natew
Copy link
Author

natew commented Dec 20, 2024

Another problem is I don't think React Native supports WASM at all, I just haven't gotten it to hit that part yet. I think even with Hermes supporting, React Native doesn't do ECMA modules yet so it simply won't let you. Seems top level still not supported as of Aug: facebook/hermes#1481 (comment)

You'd likely also want to support legacy builds a year or so back since RN isn't upgraded super often.

@aboodman
Copy link
Contributor

Let's remove it. It creates a lot of friction for various targets (cloudflare, rn, vercel). I am a bit concerned perf wise because of the hashing we do to create keys during sync, but that is temporary (we should probably change the sync protocol to not be kv at all). And we can probably get the gains back elsewhere.

@axelinternet
Copy link

Hi! Just started playing around with zerosync. React Native is the obvious use case for me so running into this issue right away created unnecessary friction to get going. Glad you are removing it and keep up the good work 👍

@aboodman
Copy link
Contributor

Moved to here: #3438

@aboodman aboodman closed this Dec 21, 2024
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.

5 participants