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

feat(js): use buffer instead of text-decoder #170

Merged

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Aug 17, 2023

  • Do not depend on fast-decoding anymore
  • added simple example app which can be used for testing.
    • NOTE: it is currently rather annoying to fully recompile aries-askar for all the android and iOS architectures so we would have to find a solution for that.

This is by no means a perfect app for testing and if we would rather wait for a better testing app I can remove it from this PR and create it later.

Ideally I would like to move to use pnpm as it makes dealing with workspaces a whole lot easier. And create some way to "quickly" (still need to compile askar like 6 times...) use custom askar builds.

@berendsliedrecht berendsliedrecht changed the title feat: use buffer instead of text-decoder feat(js): use buffer instead of text-decoder Aug 17, 2023
@andrewwhitehead
Copy link
Contributor

Looks like the workflow needs to switch to node 18

TimoGlastra
TimoGlastra previously approved these changes Aug 18, 2023
@@ -1,4 +0,0 @@
# This is a placeholder file to prevent node-pre-gyp from installing the
# aries-askar binary when cloning this repository. It won't be published to
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should not be removed I think?

Otherwise when you run yarn install it will automatically download the binaries, which you may not want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the React Native Example to be used without building it locally it is required now. It is not optimal, agreed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd fail if we increase the native binary version. As the CI will then try to install the native binary, but it doesn't exist yet

Copy link
Contributor

Choose a reason for hiding this comment

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

Being required to build it locally doesn't seem like a bad thing to me, is there a reason we don't want to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason right now is that cross only allows to be ran on a Linux host which would make building the application for Android on macOS or Windows impossible. With a better example app we could add something where you specify the platform and you can choose to only build the application for iOS which would make it work on macOS.

@andrewwhitehead
Copy link
Contributor

andrewwhitehead commented Aug 23, 2023

I'm trying to test this out but haven't gotten very far yet. A few things I've noticed (trying the ios build):

  • Commands like yarn build from the root directory produce an error, as they aren't defined by the app package
  • I had to add "uuid": "3.4.0" to the package dependencies to work around this issue: SDK45: Unable to resolve module uuid/v5 in expo go expo/expo#17261
  • I had to remove "@types/react" from aries-askar-react-native as it was producing duplicate type definitions

At the moment I'm stuck on this error. I think I need to build the iOS framework and put it somewhere, maybe under /ios?

Screenshot 2023-08-23 at 10 15 42

Edit: It looks like I can bypass this issue by running yarn ios instead of yarn expo > open iOS simulator, and then I was able to run the application.

@berendsliedrecht
Copy link
Contributor Author

I'm trying to test this out but haven't gotten very far yet. A few things I've noticed (trying the ios build):

  • Commands like yarn build from the root directory produce an error, as they aren't defined by the app package
  • I had to add "uuid": "3.4.0" to the package dependencies to work around this issue: SDK45: Unable to resolve module uuid/v5 in expo go expo/expo#17261
  • I had to remove "@types/react" from aries-askar-react-native as it was producing duplicate type definitions

At the moment I'm stuck on this error. I think I need to build the iOS framework and put it somewhere, maybe under /ios?

Screenshot 2023-08-23 at 10 15 42 Edit: It looks like I can bypass this issue by running `yarn ios` instead of `yarn expo > open iOS simulator`, and then I was able to run the application.

Hmmm, maybe I should not have added the example app just yet. Setting it up where it is easy to change the askar libraries and build it will require some more effort.

I think for now the best we can do is just to remove the example app and create a PR later with it which will include a simple script which can build the libraries and place them in the correct location.

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
@berendsliedrecht
Copy link
Contributor Author

I removed the example application as it was not ready to be merged. Will need to do some research for how we can properly set this up.

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
TimoGlastra
TimoGlastra previously approved these changes Sep 6, 2023
@berendsliedrecht
Copy link
Contributor Author

@genaris any idea what is going on here? https://github.com/hyperledger/aries-askar/actions/runs/6089025819/job/16534903694 seems to refer to your patched version of the library.

TimoGlastra
TimoGlastra previously approved these changes Sep 6, 2023
@genaris
Copy link
Contributor

genaris commented Sep 6, 2023

@genaris any idea what is going on here? https://github.com/hyperledger/aries-askar/actions/runs/6089025819/job/16534903694 seems to refer to your patched version of the library.

I think there is an issue when yarn tries to resolve types for @2060.io/ref-napi (direct dependency) and types for ref-napi (it seems that @types/ref-array-di is requiring it). Both link to the same (as we use "@types/2060.io__ref-napi": "npm:@types/ref-napi")
so yarn complains with ... is trying to unpack in the same destination "xxx" as pattern ["xxx"]. This could result in a non deterministic behavior, skipping..

In this PR, the entry for @types/ref-napi in yarn.lock was removed. If I manually re-add it, it works. Not sure yet what's the right approach here to force these types before this breaks again. Maybe we'll need to patch @types/ref-array-di as well or create an actual @types/2060.io__ref-napi in DefinitelyTyped repo?

@genaris
Copy link
Contributor

genaris commented Sep 6, 2023

@genaris any idea what is going on here? https://github.com/hyperledger/aries-askar/actions/runs/6089025819/job/16534903694 seems to refer to your patched version of the library.

Update on this: I've added type definitions to the packages in order to attempt types clashing. Changes in nodejs' package.json would look like this:

image

I've tested it locally (build, check-types, test, etc.) from a clean checkout and seems to work fine. Hope this can help to unblock this PR!

Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
@berendsliedrecht berendsliedrecht merged commit ee46c20 into openwallet-foundation:main Sep 7, 2023
26 checks passed
@berendsliedrecht berendsliedrecht deleted the use-buffer branch September 7, 2023 11:36
jamshale pushed a commit to jamshale/askar that referenced this pull request Aug 18, 2024
…e-buffer

feat(js): use buffer instead of text-decoder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants