Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
fixes #2029
fixes #2213
There was an issue with safari browser for webapp that it errors on startup with
unintiailized variable, this PR fixes that.the issue is caused by Safari bug, that it cant handle
top-level-awaitspecially when it is imported multiple times in multiple modules., for more read about this:sveltejs/kit#7805
In order to fix this issue, we need to update SvelteKit to latest version whereClientInithook feature is available, this hook allows to do some asynchronous task before the web app is loaded and rendered. and we need to remove thetop-level-awaitin orderbook wasm package build script, provide a function initialization of the orderbook wasm module and initialize it in theClientInithook.To fix this, we ended up with workaround/hack solution, which is to make a think wrapper wasm crate that initializes thejs_api(or any other big wasm crate) on start usingwasm_bindgen(start)attribute, then on the build script forjs_apiweb bindings (this is only on web/esm, no limitation for node/cjs bindings so we don't need to touch that), we need to append a custom fn that initializes thejs_apiwasm module with a try/catch, first trying sync and if it fails try async. This works because chrome error on wasm sync init when the size is over 8MB, which can be catched and then init async, but on safari the async init doesn't work at all but the sync init succeeds so we never will reach to async init.On any other browser or environment such as node with ES6, this should work fine because first the sync init is tried.
Using rust profile release flags (created
release-wasmcustom profile), we were able to reduce the wasm size down to 4.85 MB, which leaves us with plenty of room for more features that might be added in future, although if at any point we hit the 8Mb limit, we can use the wrapper solution in case Safari bug is not resolved by then.Note1: Added a simple script to check the wasm artifact size that gets executed on
checkandbuildcommand for orderbook pkg, which will throw an error if we ever hit 8MB limit.Note2: using the heavy size optimizations caused
WasmEncodedErrorandWasmEncodedResultts type bindings to be duplicated in the generated.d.ts, so needed to add a piece of script to dedupe them.Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit