-
Notifications
You must be signed in to change notification settings - Fork 143
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
[DISCUSSION] Discussing wasmThemis future here | [FIXED] wasm-themis: TypeError: TextEncoder is not a constructor; Webpack #779
Comments
Hi @maxammann! Thank you for letting us know. We are currently testing wasm-themis only in nodejs environment, and haven't seen this error before. As I understood from docs, the fix might as easy as changing the utils.js#L38 line from
to
(I'm not sure, but potentially we might also need to change the import line #L21 from However, we are not experienced with webpack, @maxammann would you mind creating a small reproducible project with Themis import? It would help us to test the fix faster. I also wanted to suggest you to try to change this line inside node_modules themis code to make sure that the fix works, but I've read this article "I want to change the code in my node_modules folder" that explains that webpack is not okay with manually tweaking node_modules even for testing. |
I'm already using patch-package (https://www.npmjs.com/package/patch-package) which would make patching it trivially. I already tried yesterday to exclude that polyfill which works fine. As we are an open source project you can checkout this project and change into the web directory: https://github.com/cyla-app/cyla-app/tree/main/web From there run: Would you be interrested in a PR with a better packaging for web for this library? There are actually different "bundle formats" which can be targeted like commonjs, umd, es6 etc which can be quite confusing. |
Sounds amazing! You mean, that you already patched that line in wasm-themis for your local build and it works fine? Can you please create a PR with this fix (for master branch), so we can make sure that other tests are passing, and merge it? 🥺👉👈
Of course, we would :) We'd be forever grateful for this, and add your name to the list of contributors and Hall of Fame :) |
TextEncoder is supported in the all browsers for a long time and also in node.js. For some node.js versions it is only available through the util package. We do not need to support browsers like IE11 because they do not support webassembly
I fixed the TextEncoder issue. In the long run we should package the node library using webpack or rollup for better comparability. I also suggest a TypeScript rewrite and then bundling it for node and browsers. |
TextEncoder is supported in the all browsers for a long time and also in node.js. For some node.js versions it is only available through the util package. We do not need to support browsers like IE11 because they do not support webassembly
Oh great, GitHub automatically closed the issue before the fix has been made available. @maxammann would you be okay with this fix released in the upcoming 0.14 (some undefined time in the future), or you need to unbreak stuff soon(ish) and would prefer some 0.13.2 to be made available? |
@ilammy Its fine if it gets released in the undefined future. Right now I plan to use a custom build for the near future as there are maybe some other things I will need to change. Did you already try whether this package works using webpack in a browser? I did not manage to load the .wasm correctly. I also do not see a way of doing it right now. I suspect that were wasn't much testing with webpack (which is fine). I plan to do a PR as soon as I manage to get it working in the browser (while keeping node suppport). Is that fine? |
Okay. So I guess “fixes for WasmThemis in browser” will be one of the highlights for 0.14, @vixentael.
Not yet :(
You're right. WasmThemis is known to be working with Node. Stable versions certainly work with Electron (and is probably working on master). But as for browsers... Let me put it like this: I think I remember that someone once told me that at some point they saw WasmThemis working in some browser. It's not tested on CI. It's not tested manually during the release (as far as I know). I wouldn't be surprised if it does not work at all, contrary to what documentation claims.
It's not only fine – it would be goddamn amazing ❤️ (if you ask me). Unfortunately, I personally don't really have the capacity at the moment for more than keeping WasmThemis on life support. Neither do other maintainers, I believe. So any help here would be much appreciated. If you fancy a TypeScript rewrite some time in the future then you have a green light from me on that. I believe that would be a valuable addition to WasmThemis. |
Makes sense for me 💪
Thank you @maxammann :) Looking forward! |
I just got it working with small changes. I copied the .js files to my project for now. There I will transfer it to TypeScript and fix issues. Afterwards I'll contribute it upstream by adding proper bundling. I added this to the makefile which makes it easier to include the wasm lib: $(BIN_PATH)/libthemis.js: LDFLAGS += -s MODULARIZE=1 |
Alright, I did the TypeScript migration: https://github.com/cyla-app/cyla-app/tree/main/web/src/themis Next step would be to run the old tests against the new bundled version :) EDIT: So this is only a minimal syntax change. I tried not to change any semantics |
I suggest to close this, as no discussions is happening right now, see #792 |
TextEncoder is supported in the all browsers for a long time and also in Node.js. For some Node.js versions it is only available through the util package. We do not need to support browsers like IE11 because they do not support WebAssembly. Fixes: cossacklabs#779 (cherry picked from commit 7d90817)
TextEncoder is supported in the all browsers for a long time and also in Node.js. For some Node.js versions it is only available through the util package. We do not need to support browsers like IE11 because they do not support WebAssembly. Fixes: cossacklabs#779 (cherry picked from commit 7d90817)
TextEncoder is supported in the all browsers for a long time and also in Node.js. For some Node.js versions it is only available through the util package. We do not need to support browsers like IE11 because they do not support WebAssembly. Fixes: #779 Co-authored-by: Max Ammann <[email protected]> (cherry picked from commit 7d90817)
Describe the bug
When importing wasm-themis in a webpack project I get the following error:
I think this is because of an incorrect usage of the fast textencoder api: https://github.com/anonyco/FastestSmallestTextEncoderDecoder#api-documentation
To Reproduce
Expected behavior
The polyfill should be used and no exception thrown.
Environment (please complete the following information):
The text was updated successfully, but these errors were encountered: