-
Notifications
You must be signed in to change notification settings - Fork 30
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
remove top level await #3427
Conversation
@natew is attempting to deploy a commit to the Rocicorp Team on Vercel. A member of the Team first needs to authorize it. |
But can’t esbuild or whoever transform the code to not use top level await |
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 |
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. |
But I agree we should probably remove xxhash. It is only used to hash that AST at this point and nowhere else. |
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. |
I’d like to remove it. Erik benchmarked it and found the wasm version
faster that’s why it remains. But it has caused a surprising amount of
problems and id rather look for the perf elsewhere. I’ll bring it up on
discord because i want to make sure there are not other things im missing.
a (phone)
…On Fri, Dec 20, 2024 at 6:33 AM Nate Wienert ***@***.***> wrote:
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.
—
Reply to this email directly, view it on GitHub
<#3427 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATUBHIAU67IPYE2JP6K732GRBEJAVCNFSM6AAAAABT6Q6RLGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJXGM2DANRRGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
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. |
A quick search shows that Hermes supports top level await. Maybe it just a matter of configuring things to not use cjs? |
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. |
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. |
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 👍 |
Moved to here: #3438 |
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.