Migrate wasm-themis to TypeScript #792

maxammann · 2021-03-26T14:39:57Z

So my PR for TS is finally here. This PR only changes syntax and should not change semantics. This is not possible in every case, but in all cases that matter.

The module works in node, older browsers and also works using ES6 and TypeScript. Here is an example for node:

mkdir test && cd test
npm init -y
npm install file://./wasm-themis-0.14.0.tgz
echo "const themis = require('wasm-themis'); \
themis.initialize().then(() => { \
    const cell = themis.SecureCellSeal.withPassphrase('pass'); \
    console.log(cell.encrypt(new Uint8Array([1]))); \
})" > example.js
node example.js

Example for web like a react app:

// @ts-ignore
import themisWasm from "wasm-themis/dist/libthemis.wasm";
import { initialize, SecureCellSeal } from "wasm-themis"; // webpack takes care of making the wasm file available

await initialize(themisWasm);
const cell = SecureCellSeal.withPassphrase(pw);

Here are links to the diffs for easier reviewing:

Syntax changes from plain JS to TS

You can simple review the commit referenced above and then only review the changes to the package.json and build files.

Checklist

Change is covered by automated tests
Benchmark results are attached (if applicable)
The [coding guidelines] are followed
Public API has proper documentation
Example projects and code samples are up-to-date (in case of API changes)
Changelog is updated (in case of notable or breaking changes)

That way we can use the TS default values in function calls

maxammann · 2021-03-26T14:42:07Z

I'm not sure where to change example or where examples exist. The api only changed ìnitialisedto ìnitialize()

maxammann · 2021-03-26T14:44:02Z

src/wrappers/themis/wasm/src/libthemis.d.ts

@@ -0,0 +1,236 @@
+import { ThemisErrorCode } from "./themis_error";


This file was crafted manually and describes the type of libthemis.js

maxammann · 2021-03-26T14:44:54Z

src/wrappers/themis/wasm/src/secure_cell_context_imprint.ts

+  }
+
+  encrypt(message: Uint8Array, context: Uint8Array) {
+    message = coerceToBytes(message);


In a future PR I would like to remove all usages of coerceToBytes and demand a Uint8Array to be passed to the library. That would simplify the code in many places.

maxammann · 2021-03-26T14:45:16Z

src/wrappers/themis/wasm/test/test.js

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and


I left test.js as is. That way I could verify that I did not break anything.

maxammann · 2021-03-26T14:45:48Z

src/wrappers/themis/wasm/test/test.js

@@ -36,14 +36,14 @@ function measureTime(thunk) {

 describe('wasm-themis', function() {
    let generallyInvalidArguments = [
-        null, undefined, 'string',


undefined means no argument in TS and ES6. Therefore it is an valid argument.

maxammann · 2021-03-26T14:46:08Z

src/wrappers/themis/wasm/wasmthemis.mk

@@ -26,6 +26,7 @@ WASM_PACKAGE = $(BIN_PATH)/wasm-themis.tgz

 $(BIN_PATH)/libthemis.js: LDFLAGS += -s EXTRA_EXPORTED_RUNTIME_METHODS=@$(WASM_RUNTIME)
 $(BIN_PATH)/libthemis.js: LDFLAGS += -s ALLOW_TABLE_GROWTH
+$(BIN_PATH)/libthemis.js: LDFLAGS += -s MODULARIZE=1


That is needed for web. This allows me to configure the path to the wasm file.

maxammann · 2021-03-26T15:10:17Z

I think the RustThemis failure is not introduced in this PR. For some reason Node 10 fails because of a timeout when initializing wasm. Not sure why this happens...

maxammann · 2021-03-26T16:13:08Z

src/wrappers/themis/wasm/src/secure_keygen.ts

-export class PrivateKey extends Uint8Array {
+export class PrivateKey {


I had to remove the extends. Officially it is not supported in ES5 to extend buildins. I want to target ES5 instead of ES6 by default to be compatible with the majority of browsers.

This led to some minor changes in tests.

maxammann · 2021-03-26T16:14:02Z

src/wrappers/themis/wasm/test/test.js

-        if (error instanceof themis.ThemisError) {
+        if (error.errorCode) {


instanceof is also not supported in ES5. Currently tests run with ES5 to catch comparability issues.

maxammann · 2021-03-26T16:22:01Z

Next steps:

Remove tests which check for types errors
Remove all exceptions which are thrown because of wrong types. Typescript handles this now.
Remove legacy arguments.length == handling
Remove coerceBytes
code formatting

vixentael · 2021-03-26T18:33:07Z

Wow, that's a lot 🔥

maxammann · 2021-03-26T21:14:38Z

Wow, that's a lot 🔥

Yeah I tried to do minimal changes. But a rewrite is a rewrite :D

The other changes can follow in other prs. They are optional and just code quality improvements.

ilammy

Overall, I'm hugely positive for this changeset. More type-level static guarantees with acceptable usability is better. I'm especially happy that someone actually experienced with JavaScript gave at look at it. I know how tedious it might have been, so props from me, Max! ❤️

ilammy · 2021-03-27T06:17:29Z

src/wrappers/themis/wasm/src/index.ts

+export const initialize = async (wasmPath: string) => {
+  const libthemis = await libthemisFn({
+    onRuntimeInitialized: function () {},
+    locateFile: function () {
+      return wasmPath;
+    },
+  });


Regarding the formatting change from "no semicolons and 4-space indents with single quotes" to "use semicolons with 2-space indents and double quotes". Most of the codebase uses 4-space indents, unless there is a strong preference for a particular style in the language (e.g., tabs in Go, 2-space indents in Ruby). Is there such thing in TypeScript ecosystem as a whole?

I don't really mind the style changing, given that all these lines are still going to change anyway with added typing and strict checks. But it would be nice to make it less of a cowboy and more organized. It would be cool to throw in code formatter to ensure that all newly added code is consistent with the new style. (Not in this PR. But maybe some time later.)

(Admittedly, existing JavaScript code is a mess right now. JsThemis tests, for example, use 2, 4, 8-space indents and a bunch of tabs in different places.)

I agree, I already throught about throwing in prettier.

Pretter works across IDEs and is the defacto standard for formating. I add this to the todo list.

ilammy · 2021-03-27T06:29:20Z

src/wrappers/themis/wasm/package.json

+    "@types/emscripten": "^1.39.4",
+    "@types/node": "^14.14.36",


How strictly those should track the version of Emscripten used to compile WasmThemis? (System Node.js is less important since Emscripten uses its own, IIRC.)

Emscripten APIs don't change often, but recently they did. I guess we should just try updating those from time to time, see if it builds and we're good?

I think we should change the version of the types if we upgrade emscripten. I am not sure who creates the types but i hope it is somebody who is involved in the emscripten project.

The cool thing is, if we upgrade types here and there were api changes, then we will get compile time errors.

ilammy · 2021-03-27T06:50:43Z

src/wrappers/themis/wasm/test/test.js

    describe('initialization', function() {
        it('resolves "initialized" promise', function(done) {
-            themis.initialized.then(function() {
+            themis.initialize().then(function() {


@vixentael, how strict are we on backwards compatibility in WasmThemis? Does the usual “we do not break userspace” apply here as well?

In my opinion, unless it's unbearable and makes maintenance a hell, Themis should strive to stick to once exported APIs and avoid breakage. From what I have seen so far, there are the following changes:

Most APIs now accept strictly Uint8Array, disallowing ArrayBuffer (in TypeScript).

Overall, I'm okay with making input types more strict – don't really remember the story behind coerceToBytes(). The tests don't look like they test this behavior either.

Since there was no TypeScript support before, TypeScript users will just be using Uint8Array everywhere from the day 1.

Vanilla JavaScript users will not be affected by this for now: existing coerceToBytes() calls will check types at runtime and convert ArrayBuffer into Uint8Array. TypeScript won't insert extra runtime checks that would block that. However, once coerceToBytes() calls are gone, using ArrayBuffer will become an error.

@maxammann, is my reasoning correct so far?

If so, I guess it would be nice to add some runtime warning when ArrayBuffer is passed to WasmThemis API, notifying JavaScript users that support for this will soon be removed, please cast to Uint8Array yourself.

initialized promise => initialize() async function.

Ideally, I'd like to get rid of this thing entirely as it serves no useful purpose other than making a feeble sacrifice to the God of async. But I guess not today... I'm not really sure there is a better way other than inserting a check & wait for initialization into every exported public function. Or providing an entirely new async API.

However, I believe it's not be nice to snatch the rag from existing users and remove initialized entirely. Would it be possible for initialized and initialize() to coexist for a while?

Changing the base type of private/public keys.

Now they are not Uint8Array by themselves, you should use the .data field to access key date. This might break user code that stores, encrypts, and otherwise processes keys.

Is there maybe some way to keep inheriting from Uint8Array for ES6 code, but provide a data accessor for ES5 compatibility? (I believe WasmThemis supported only ES6 before...)

(And if we're going to make any breaking changes, they should be communicated and documented in the changelog along with migration path.)

@maxammann, is my reasoning correct so far?

Perfectly correct!

However, I believe it's not be nice to snatch the rag from existing users and remove initialized entirely. Would it be possible for initialized and initialize() to coexist for a while?

I will wee whether that is possible. I can not start loading themis as soon as this library is importet as you may want to configure the path to the wasm binary (required for web and webpack).

Changing the base type of private/public keys.

Supporting es5 is not possible with that extends syntax unfortunately. I had to support and target es5 such that the integration CI passes. So I think we should support it. The reason why it worked before is that without a compilation it somehow worked as node supports a mix between es5 and es6. Some browsers do not and only support es5. Therefore this kind of fixed bugs for users of older browsers.

BUT: We could drop support for ES5 actually because every browser which supports webassambly should already support ES6. This is also the reason why we do not need to support IE11. Webassambly does not work there in the first place.

Perfectly correct!

Whew 😌 Thanks for confirming.

I will wee whether that is possible. I can not start loading themis as soon as this library is importet as you may want to configure the path to the wasm binary (required for web and webpack).

We have not really considered – and tested – the web usage. Knows users of WasmThemis are all on Node.js.

I remember doing some manual tests and that it kinda worked. But I'm no web developer so I believe no one so far has tested WasmThemis with proper web apps. Hence, I wouldn't be really surprised that it does not work out of the box. If something is necessary for proper web support – go ahead, you have my blessing to add whatever is needed 👍

It would be nice to keep the initialized promise around – I'm sorry, I don't have any ideas right now – but it it's not possible, then, well, WasmThemis users are getting a flag day. This one should not be too painful to work around.

BUT: We could drop support for ES5 actually because every browser which supports webassambly should already support ES6.

Uh-huh... I faintly remember this reasoning behind setting the minimal supported version to ES6 and using all those fancy things.

Personally, I'm fine with ES5 support being added, but probably not at an expense of backwards-incompatible changes to the API which are caused by ES5 lack of features. If there was a way to give ES5 users one code and ES6 gets a different one – that would be great, but browsers don't work like that: it's easier to serve the same version to everyone without trying to guess what their browser might or might not support. So the approach would be something like "use some magic transpiler to convert ES6 code into ES5-compatible code".

src/wrappers/themis/wasm/test/test.js

ilammy · 2021-03-28T03:05:50Z

Also, a comment from the operational point of view for @shadinua: WasmThemis publishing should not really change much with all of this. You can still just run make wasmthemis to build a wasm-themis.tgz to be uploaded to npm and that's it. The build process gets slower though, as now it needs to download more stuff from the Internet, transpile TypeScript into JavaScript, etc.

ilammy · 2021-03-29T12:54:42Z

FYI, @maxammann, the fix for Rust build has been merged into master, so you can pull the updated version and merge it into your branch to turn the build green. Just a merge commit with git checkout typescript && git merge master, not rebases or anything like that (the PR branch is going to get squashed anyway).

maxammann · 2021-04-17T08:29:53Z

@ilammy any update on this one? :)

ilammy · 2021-04-18T03:24:34Z

@maxamman, I'm sorry, it seems that high-context culture has some effects on me and I kinda mishandled this.

It looks like we ended up playing this silly game where everybody expects other people to do something, but they never tell that explicitly and the other party does not read the air. And so weeks pass without anyone doing anything while everybody is under impression something is being done and it just takes time. From my point of view, open-source contributors are volunteers and all, they have other stuff to keep them busy. From your point of view, I think, maintainers might look the same.

I'm biased for merging this to make future work easier. However, this will block the 0.17.0 release until the backwards compatibility issues outlined in #792 (comment) are resolved (one way or another: either we avoid them, or we assert them). I'm not aware of time constraints for this release, so I guess I'd go for making it when it's done rather than postponing TypeScript rework for some later release.

@vixentael, what's your take on this?

Again, @maxammann, I'm sorry for not setting my expectations straight 🙏 I was under impression that you were looking into those issues, it just takes time.

maxammann · 2021-04-18T08:13:04Z

I'm sorry, it seems that high-context culture has some effects on me and I kinda mishandled this.

Absolutely no problem, no mishandling happened in my opinion :)

Actually, I did not see your last comment in our discussion. As far as I can see I have two open things:

Continue to support initialized promise
Drop ES5 support because all wasm supporting browsers also support ES6 and therefore also revert the changes about extending ArrayBuffer

vixentael · 2021-04-18T22:25:47Z

Thank you for your patience @maxammann!

Scenario 1

Continue to support initialized promise

Would be nice if we can!

Drop ES5 support

I'm ok with this., it's old :)

If we can support old initialized promise OR write deprecation notice on that one, then I think we can merge TS support to master and release with next Themis release.

Scenario 2

If we can't support old initialized promise, then I suggest merging these changes to a separate branch master/typescript, and aim at releasing 0.14.0 with breaking changes (add initialize(); drop ES5) :)

I'd love to support backwards compatibility, but I understand that WASM Themis is typically used in desktop and web apps, which move fast. Soo... sorry devs? We will describe breaking changes as detailed as possible.

@maxammann do you think it's possible to support initialized promise or to write some deprecation/breaking change notice in code to let devs know that they should use initialize now?

ilammy · 2021-04-19T12:46:31Z

Alrighty. @maxammann, I've retargeted your PR onto a new branch – wasm-typescript – and merged it there. Please target your future TypeScript PRs for that branch. Once TypeScript support is stabilized and ready for release, the wasm-typescript branch will be merged into master.

This way if Themis really needs a release, we can provide it without potentially introducing a breaking change. Hopefully, we won't need this and TypeScript will be in there in Themis 0.14.

maxammann · 2021-05-02T14:02:42Z

Thanks! Right now I don't have time to continue but soon I will :)

Just ping me when there are urgent changes for wasm-themis.

maxammann added 11 commits March 26, 2021 13:48

Modularize wasm themis

e5cc867

Change extensions

f3a1a89

Translate plain js to typescript

9336d39

Add typescript and esbuild dependencies

dada873

Add tsconfig and script to build library

f17c73c

Fix tests by making undefined a valid value

81a2d25

That way we can use the TS default values in function calls

Add ts-mocha package

ccfa90f

Make wasmPath optional for node environment

232d64e

Fix gitignore

d14fcd5

Move declaration into sepratate file

f7113d6

Remove esbuild and setup library creation

be2ad37

maxammann marked this pull request as ready for review March 26, 2021 14:43

maxammann requested review from ilammy, shadinua and vixentael as code owners March 26, 2021 14:43

maxammann commented Mar 26, 2021

View reviewed changes

ilammy added the W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages label Mar 26, 2021

maxammann commented Mar 26, 2021

View reviewed changes

maxammann added 4 commits March 26, 2021 15:50

Run npm install

b250394

Update lock file

d1a3f3c

Switch target to es6, else tests fail

2da6cb9

Fix compile commands

70712b9

maxammann mentioned this pull request Mar 26, 2021

[DISCUSSION] Discussing wasmThemis future here | [FIXED] wasm-themis: TypeError: TextEncoder is not a constructor; Webpack #779

Closed

2 tasks

maxammann added 2 commits March 26, 2021 16:13

Update node code in tools/

d50be05

Set target for tests to es5

3403cbc

maxammann added 2 commits March 26, 2021 17:10

Remove extends; this is not allowed with ES5

efd516b

Fix tests for adapted api and es5

d2147b9

maxammann commented Mar 26, 2021

View reviewed changes

ilammy reviewed Mar 28, 2021

View reviewed changes

maxammann added 3 commits March 30, 2021 12:30

Merge branch 'master' into typescript

ac0bf2d

Readd test

b49c385

Upgrade package

a4e84ee

ilammy approved these changes Apr 18, 2021

View reviewed changes

vixentael mentioned this pull request Apr 18, 2021

WASM Failure on Safari on macOS and iOS #798

Closed

1 task

ilammy changed the base branch from master to wasm-typescript April 19, 2021 12:39

ilammy merged commit 07f6168 into cossacklabs:wasm-typescript Apr 19, 2021

ilammy mentioned this pull request Jul 11, 2021

Bring back initialized promise #854

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate wasm-themis to TypeScript #792

Migrate wasm-themis to TypeScript #792

maxammann commented Mar 26, 2021 •

edited

Loading

maxammann commented Mar 26, 2021

maxammann Mar 26, 2021

maxammann Mar 26, 2021

maxammann Mar 26, 2021

maxammann Mar 26, 2021

maxammann Mar 26, 2021

maxammann commented Mar 26, 2021

maxammann Mar 26, 2021

maxammann Mar 26, 2021

maxammann Mar 26, 2021

maxammann commented Mar 26, 2021 •

edited

Loading

vixentael commented Mar 26, 2021

maxammann commented Mar 26, 2021

ilammy left a comment

ilammy Mar 27, 2021

maxammann Mar 28, 2021

ilammy Mar 27, 2021

maxammann Mar 28, 2021

ilammy Mar 27, 2021 •

edited

Loading

maxammann Mar 28, 2021

ilammy Mar 29, 2021

ilammy commented Mar 28, 2021

ilammy commented Mar 29, 2021

maxammann commented Apr 17, 2021

ilammy commented Apr 18, 2021

maxammann commented Apr 18, 2021

vixentael commented Apr 18, 2021

ilammy commented Apr 19, 2021

maxammann commented May 2, 2021

		@@ -0,0 +1,236 @@
		import { ThemisErrorCode } from "./themis_error";

		@@ -12,7 +12,7 @@
		// See the License for the specific language governing permissions and

		export class PrivateKey extends Uint8Array {
		export class PrivateKey {

		if (error instanceof themis.ThemisError) {
		if (error.errorCode) {

Migrate wasm-themis to TypeScript #792

Migrate wasm-themis to TypeScript #792

Conversation

maxammann commented Mar 26, 2021 • edited Loading

Checklist

maxammann commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxammann commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxammann commented Mar 26, 2021 • edited Loading

vixentael commented Mar 26, 2021

maxammann commented Mar 26, 2021

ilammy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilammy Mar 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilammy commented Mar 28, 2021

ilammy commented Mar 29, 2021

maxammann commented Apr 17, 2021

ilammy commented Apr 18, 2021

maxammann commented Apr 18, 2021

vixentael commented Apr 18, 2021

Scenario 1

Scenario 2

ilammy commented Apr 19, 2021

maxammann commented May 2, 2021

maxammann commented Mar 26, 2021 •

edited

Loading

maxammann commented Mar 26, 2021 •

edited

Loading

ilammy Mar 27, 2021 •

edited

Loading