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

chore: all setup #15

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Jan 15, 2025

  • Only include the JS wrapper
  • Remove eslint+prettier+jest and replaces with biome+node:test
  • Update wrapper functionality to latest released askar (0.4.1)
  • Make example app working again

Berend Sliedrecht and others added 5 commits January 15, 2025 16:44
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
@swcurran
Copy link
Contributor

Hey @berendsliedrecht — awesome to see this. Thanks!

Will you be working on sorting out the Android build tests and if so, will you need help in how best to get them going? I gather it is a bit of dependency hell…

@jleach @cvarjao — please review this PR and do what you can to help get this completed.

Signed-off-by: Berend Sliedrecht <[email protected]>
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Very nice!

Will we do the version update in a follow-up PR?

with:
name: library-${{ matrix.architecture }}
path: target/${{ matrix.target }}/release/${{ matrix.lib }}
- run: ls -R packages/askar-nodejs/native
Copy link
Contributor

Choose a reason for hiding this comment

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

I try to use corepack everywhere now.

corepack use pnpm@v9.??.?

Will be picked up by this action as well

or [https://opensource.org/licenses/MIT](https://opensource.org/licenses/MIT))

at your option.
# Askar Wrapper JavaScript
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe copy part of the specific wrappers readme? Or from Credo as a template? Would be good to at least have something

| Askar | JavaScript Wrapper |
| ----------- | ------------------ |
| v0.2.9 | v0.1.0, v0.1.1 |
| v0.3.1 | v0.2.0 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Also should be added to react native file

Suggested change
| v0.3.1 | v0.2.0 |
| v0.3.x | v0.2.x |
| v0.4.1 | v0.3.x |

@berendsliedrecht
Copy link
Contributor Author

Very nice!

Will we do the version update in a follow-up PR?

I have spend the last two days working on trying to get Android working with RN 76. Ios works without any issues, but Android is way more complex sadly. I am not sure when I have the time to look into it again. For now everything works with RN up to 75 and expo 52.

@TimoGlastra
Copy link
Contributor

Very nice!
Will we do the version update in a follow-up PR?

I have spend the last two days working on trying to get Android working with RN 76. Ios works without any issues, but Android is way more complex sadly. I am not sure when I have the time to look into it again. For now everything works with RN up to 75 and expo 52.

I actually meant updating the version of the wrapper, but supporting RN 0.76 would be great to! :)

What are the issues with RN 0.76 for Android? Maybe you can open an issue so it's clear what is not working?

@berendsliedrecht
Copy link
Contributor Author

Very nice!
Will we do the version update in a follow-up PR?

I have spend the last two days working on trying to get Android working with RN 76. Ios works without any issues, but Android is way more complex sadly. I am not sure when I have the time to look into it again. For now everything works with RN up to 75 and expo 52.

I actually meant updating the version of the wrapper, but supporting RN 0.76 would be great to! :)

What are the issues with RN 0.76 for Android? Maybe you can open an issue so it's clear what is not working?

When I set everything up it all works as it is supposed to (I can call c++ functionality from js with codegen and all), but when I add a shared object (i.e. libaries_askar.so) it complains about not being able to find another turbo module for PlatformConstants. I will create an issue for it later.

@swcurran
Copy link
Contributor

An issue would be a big help, so that we can get someone else to take it. A summary of where you are, where you are stuck, and next steps after getting unstuck would be ideal. Sorry this is so painful!

@TimoGlastra
Copy link
Contributor

I guess react native mmkv also uses a c library, and turbo modules? I guess we could look at that @berendsliedrecht?

@berendsliedrecht
Copy link
Contributor Author

I guess react native mmkv also uses a c library, and turbo modules? I guess we could look at that @berendsliedrecht?

React native mmkv does not use a precompiled shared object, they compile their code using the same CMakeLists.txt file as building their entire react native mmkv library. We link an additional shared object file in later and when I do that it fails.

This PR can still be merged, but it will work for Android RN up to 75 and iOS up to latest. (so no android RN 76 support only).

It might also be possible to not use codegen and a lot of new features and make it work, but I have not had the time yet.

@TimoGlastra
Copy link
Contributor

Okay, if you can address the feedback we can merge this and tackle android 0.76+ later 👍

@swcurran
Copy link
Contributor

FYI — @cvarjao @jleach

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.

Update this repo to be just the Askar JavaScript Wrapper and create a release pipeline
3 participants