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: updated to fetch 0.4.2 binary #270

Closed
wants to merge 5 commits into from

Conversation

wadeking98
Copy link
Contributor

Updated the javascript wrappers to use the 0.4.2 binary

Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

I think it is good to put the efforts in the main branch but we just need to make sure that it works properly in both nodejs and react-native, as mentioned in #268 (comment). I didn't follow the latest releases (other than some minor fixes) but I guess the js-0.4.1 branch was created for a compatibility issue so it would be nice to get feedback from @TimoGlastra or @berendsliedrecht.

Besides that, I remember there were several methods not implemented in the wrappers even for 0.4.1, so it would be good also to take a look if they are already there now or they need to be added before doing another official release: see #216 (comment)

| ------------- | ------------------ |
| v0.4.0-dev.16 | v0.1.0 |
| v0.4.1 | v0.2.0 |
| v0.4.2 | v0.2.3 |
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 will be clearer if we explicitly mention that v0.4.1 corresponds to 0.2.0, 0.2.1 and 0.2.2, or just 0.2.x and call this one (the one under development) as 0.3.0.

Actually in any case I'd prefer to move on to 0.3.0

@swcurran
Copy link
Member

swcurran commented May 8, 2024

I’d love to get this into a single branch and to have unified releases of all components at the same time. Additional overhead, but pretty important!

@wadeking98
Copy link
Contributor Author

@genaris is there an easy way to test it on node and react native? from my understanding it only pulls the binaries when you run npm install from the npm repository.

@@ -1,3 +0,0 @@
# This is a placeholder file to prevent node-pre-gyp from installing the indy_vdr binary when cloning
Copy link
Member

Choose a reason for hiding this comment

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

This needs to stay!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry about that, I was testing some things in nodejs and didn't mean to commit it

@TimoGlastra
Copy link
Member

@berendsliedrecht maybe we can also add the example app to this branch so we can more easily test changes in react native?

@TimoGlastra
Copy link
Member

I’d love to get this into a single branch and to have unified releases of all components at the same time. Additional overhead, but pretty important!

I think that makes sense, however it means we will need to require every pull request to implement functionality for all wrappers. It's sometimes quite hard to look in which way a wrapper is outdated, as you have to walk through all commits, see the changes, update it, test the different wrappers, etc..

@wadeking98
Copy link
Contributor Author

I made some changes to the binary to make it work with react native. For some reason the #[async_trait] macro wasn't working well with react native so I removed it and refactored the code to compensate. I'm going to split up this PR to update the binary and one to update the wrapper

@wadeking98 wadeking98 marked this pull request as draft May 15, 2024 18:57
@TimoGlastra
Copy link
Member

I think this PR can be closed right @wadeking98? I made #296 with updates and a new version for the JS wrapper

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.

5 participants