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

Merge branch wasm-typescript #862

Merged
merged 10 commits into from Jul 19, 2021
Merged

Merge branch wasm-typescript #862

merged 10 commits into from Jul 19, 2021

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Jul 16, 2021

Finally, TypeScript rewrite is ready to be merged with no breaking changes.

Pulling in some hotfixes from release/0.13 introduces some more merge conflicts, and GitHub UI is as bad and useless at them as ever. This time it can at least offer to resolve them, but I don't want more “samba merges” in the history. So yet another manual merge.

Most of these changes come from #729, with some follow-ups. Note that there are no changes in docs/examples/wasm and tools/js/wasm-themis – and they still work. Also, no tests in src/wrappers/themis/wasm/test/test.js were removed and they still work. I have also manually checked the webpack example, it still works.

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has proper documentation
  • Example projects and code samples are up-to-date
  • Changelog is updated
  • Just push when approved, don't use GitHub UI

maxammann and others added 10 commits April 19, 2021 15:43
This adds initial support of TypeScript for WasmThemis.
There are some (yet undocumented) breaking changes
which might actually be resolved later.

* Modularize wasm-themis
* Translate plain js to typescript
* Add typescript and esbuild dependencies
* Add tsconfig and script to build library
* Fix tests by making undefined a valid value
* Remove extends; this is not allowed with ES5
* Upgrade node package version
Conflicts in "wasm/package-lock.json" have been resolved by merging
changes from both branches.

There were also conflicts in "tools/js/wasm-themis" as 'wasm-typescript'
branch has introduced some breaking changes in API. I've reconciled
the changes between branches, favoving the "broken" API here. I've also
updates other WasmThemis examples to use this API.

The plan is to get the CI running, helping with this, and then work on
removing backwards-incompatible changes in later commits.
Previous change has removed it from the list of 'invalid values' because
in TypeScript "undefined" is interpreted as literally "omitted argument"
which is actually allowed in some places and does not cause failures.

However, test tests are actually written in JavaScript, so we can pass
"undefined" there just fine.

Instead of removing "undefined", skip it for those APIs that accept
omitted arguments, but still test other APIs with "undefined".

This brings WasmThemis test coverage more in sync with master branch.
* Bring back "initialized" promise with some vile hacks

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"

* Use "initialized" in tests and examples once again

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.)

* Dedicated initialization tests

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.

* Mention initialize() in the changelog

We're going through with this API, so let's tell the users that it's
available now.
WasmThemis initialization fills in the "context.libthemis" singleton
that is actually used by all WasmThemis functions. That is, calling
initialize() multiple times is technically permitted, but it might not
have an effect that you expect. WasmThemis will always use WebAssembly
context from the last initialization.

While reinitialization might be useful, it does not seem to be now.
Let's allow calling initialize() only once. (This includes calling it
indirectly via the "initialized" promise.) We can always allow it later,
once the semantics of this are clarified.
* Run unit tests against ES6 compiled code

Instead of running them for ES5 target, use ES6. However, with CommonJS
modules still, because tests are written in that style.

* Compile ES6 with CommonJS modules instead of ES5

That's right. Instead of compiling for genuine ES5, compile TypeScript
into ES6-style code but as a CommonJS module. This allows using things
like require() in a suitable environment.

Initially, WasmThemis has been distributed like this, and that's what we
keep for non-ES6-module consumers.

Dropping ES5 compilation opens up a bunch of things that we can now do
for restoring compatibility with previously released WasmThemis versions.

* SymmetricKey, PublicKey, PrivateKey are Uint8Arrays once again

Instead of keeping a "data" field, extend Uint8Array directly, like
these classes used before. Now that we don't have to go out of our way
to accommodate ES5 code that cannot correctly extend Uint8Arrays.
(Way to go, ECMAScript standards group!)

* Verify key types in tests

Now that we have restored compatibility, add type tests to ensure
that whatever these functions return matches the expected type and
is a genuine Uint8Array.

This is a stronger message "think twice before silently breaking
compatibility".
That's where this file is located in WasmThemis 0.13.X and some users
might have hardcoded its location (like it's hardcoded in WasmThemis
example with webpack, for example). Let's package the original copy
of libthemis.wasm at its original path, for compatibility.

This makes installed, unpackaged size a bit larger, but JavaScript
developers are used to "node_modules" being heavier then their mom
so it's okay. And this does not affect downloads, since identical
files are compressed with deduplication.
* Provide default export from the main module

Because WasmThemis 0.13.X has been using CommonJS modules, various
transpilers interpreted

    import themis from 'wasm-themis';

as a valid way to import WasmThemis stuff under "themis" namespace
in genuine ES6 code.

The "correct" way to do this is as follows:

    import * as themis from 'wasm-themis';

but we need to make sure that the previous form still works.

It's awkward, but when did "it looks awful" was an arguments against
making a breaking change?

* Drop unnecessary "secure_cell.ts"

"index.ts" exports all Secure Cell variations individually and this file
is not really used anywhere else. File structure of WasmThemis is
implementation detail. You're not supposed to peek into it.
I think the author warrants a credit here and the users have a right to
know that TypeScript is available (as well as the first-class support of
ES6 modules).
@ilammy ilammy added the W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages label Jul 16, 2021
@ilammy ilammy requested review from vixentael and a team July 16, 2021 08:34
@ilammy ilammy requested a review from shadinua as a code owner July 16, 2021 08:34
Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

We shall update product dos as well

@ilammy ilammy merged commit 8338929 into cossacklabs:master Jul 19, 2021
@ilammy ilammy added this to the 0.14.0 milestone Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants