-
Notifications
You must be signed in to change notification settings - Fork 144
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
Bring back initialized
promise
#854
Merged
ilammy
merged 5 commits into
cossacklabs:wasm-typescript
from
ilammy:typescript-initialized
Jul 14, 2021
Merged
Bring back initialized
promise
#854
ilammy
merged 5 commits into
cossacklabs:wasm-typescript
from
ilammy:typescript-initialized
Jul 14, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Historically, WasmThemis has exported only the "initialized" promise. WasmThemis was not modularized and kicked off "libthemis.wasm" download right after the module is loaded. Moreover, there was no way to use a different path to WebAssembly code: it had to be located in a file named "libthemis.wasm" next to the JavaScript code bundle. TypeScript rewrite introduced an alternative interface "initialize()" which allows to specify the URL. However, this required WasmThemis modularization. That is, WasmThemis *will not* be initialized unless initialize() is called. This introduces two issues with backwards compatibility: 1. "initialize" promise was gone. 2. You can no longer expect that WasmThemis will be downloaded in background without awaiting any promise. Due to modularization, there is no (practical) way to fix the second issue. Just don't do, it's a bad practice. However, absence of "initialized" is a real breaking change that will break well-formed applications. Reintroduce the "initialized" promise and make sure it works by calling the "initialize()" function behind the scenes. It's a bit of a hack, but it works, so whatever. Note that "initialize" is *not* deprecated or anything. It's a perfectly valid API if you don't need to specify a custom path to "libthemis.wasm"
Revert previous changes that made all examples and tests use "initialize()" due to absence of "initialized". Now that it's there, let's use the old thing. (And this also ensures that it exists and working.)
Add a bunch of tests to verify initialization specifically. Each of these has to be a separate file, separately tested by an individual mocha process (becaue you can only initialize WasmThemis once). The "invalid path" test prints out some errors to stderr. This is default behavior for Emscripten shim. It can be overriden with more parameters to libthemisFn(), but I'm not ready to introduce too much customizability to it yet.
We're going through with this API, so let's tell the users that it's available now.
5 tasks
Lagovas
approved these changes
Jul 13, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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.
Restoring compatibility, one step at a time.
Background
Historically, WasmThemis has exported only the
initialized
promise. WasmThemis was not modularized and kicked offlibthemis.wasm
download right after the module is loaded. Moreover, there was no way to use a different path to WebAssembly code: it had to be located in a file namedlibthemis.wasm
next to the JavaScript code bundle.TypeScript rewrite introduced an alternative interface
initialize()
which allows to specify the URL. However, this required WasmThemis modularization. That is, WasmThemis will not be initialized unlessinitialize()
is called.This introduces two issues with backwards compatibility:
initialize
promise was gone.Due to modularization, there is no (practical) way to fix the second issue. Just don't do, it's a bad practice.
However, absence of
initialized
is a real breaking change that will break well-formed applications.Reintroduce the
initialized
promise and make sure it works by calling theinitialize()
function behind the scenes. It's a bit of a hack, but it works, so whatever.Note that
initialize
is not deprecated or anything. It's a perfectly valid API if you don't need to specify a custom path tolibthemis.wasm
Initialization APIs
So basically, now WasmThemis has two initialization APIs.
Historically first:
which will download
libthemis.wasm
relative to the executing JS bundle.Explicit — available since WasmThemis 0.14.0:
which allows you to specify the exact URL for downloading WebAssembly code.
Maybe later it will accept more parameters that can be passed to Emscripten (see their docs), but I don't want to expand the API unless there are users for it.
Ancillary changes
Existing unit tests, examples, and tools are rolled back to using
initialized
.New tests verify the all initialization APIs work as expected.
Checklist
master
intowasm-typescript
#845 is merged