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

Update C++ code #32

Merged
merged 1 commit into from
Apr 2, 2024
Merged

Conversation

boorad
Copy link
Collaborator

@boorad boorad commented Feb 21, 2024

There are a few bug fixes/improvements in Kevin Heifner's fork of cpp-base64. This project was using 2.rc.7, and the fork has the following changes:

  • based from 2.rc.09
    • with a bug fix allowing for unpadded strings
  • modified to be header-only
  • templated for various string / char containers
  • performance improvements (decoding via lookup table)
    • speed improvements measured at 4x-6x over v2.0.8

This PR:

  • updates the project to use this fork.
  • updates the example app from RN 0.66.3 to 0.73.6
  • adds a test suite (from base64-js and node's crypto)

@boorad boorad marked this pull request as draft February 21, 2024 17:33
@boorad boorad marked this pull request as ready for review February 21, 2024 17:38
@boorad
Copy link
Collaborator Author

boorad commented Feb 21, 2024

Android works, but is no faster than base64-js on my Pixel 8 Pro. Any ideas why?

@craftzdog
Copy link
Owner

base64ToArrayBuffer is undefined when running on Android. So, it seems to be not initialized properly.
Can you look into op-sqlite's implementation?

@boorad
Copy link
Collaborator Author

boorad commented Feb 22, 2024

Yes, will do. Also, these changes are not yet solving the issue for my PRs in react-native-quick-crypto, so I may search for and add some more tests. Something is still a bit off...

@boorad
Copy link
Collaborator Author

boorad commented Mar 21, 2024

@craftzdog I cannot for the life of me figure out what I broke in this branch. Unable to resolve module react-native-quick-base64 so I can't run it on either ios or android right now. I pushed my changes based on what I saw in rn-quick-crypto, op-sqlite, and rn-vision-camera. Any ideas?

@craftzdog
Copy link
Owner

I'll look into it next month or so.

@boorad
Copy link
Collaborator Author

boorad commented Mar 21, 2024

Found the issue. Pressing on to fix the original Android problem.

@boorad
Copy link
Collaborator Author

boorad commented Mar 24, 2024

Changes to the Example App:

  • add test suite results
  • clean up benchmarks
  • overall layout

Simulator Screenshot - iPhone 15 Pro - 2024-03-24 at 13 24 28
Screenshot_20240324_134601

@boorad boorad marked this pull request as draft March 24, 2024 17:50
@craftzdog
Copy link
Owner

It works in my environment!

Screenshot 2024-03-27 at 23 53 52

@boorad
Copy link
Collaborator Author

boorad commented Mar 27, 2024

I'm waiting on any required changes to this branch that might be needed in react-native-quick-crypto PR's before asking you to review and merge. I'm stuck on a bad jwk key import, but hopefully will be past that issue soon. Then we can cut a v2.1.0 release.

@boorad boorad force-pushed the feat/heifner-version branch from b434131 to 0e96f0c Compare March 27, 2024 16:02
@boorad
Copy link
Collaborator Author

boorad commented Mar 27, 2024

@craftzdog any idea what's going on with the GHA builds?

@craftzdog
Copy link
Owner

yeah, strange. I tried clearing the GitHub cache but it still doesn't work

@boorad
Copy link
Collaborator Author

boorad commented Mar 29, 2024

hmm, you didn't get my squashed commit (force-push)... I think I can fix it later after the GHA stuff works.

@craftzdog
Copy link
Owner

made the iOS GHA build successful but still no luck with the Android build.
maybe it'd be helpful to look into rn-mmkv's latest code:

mrousavy/react-native-mmkv@94522bd

@craftzdog
Copy link
Owner

@boorad boorad force-pushed the feat/heifner-version branch 2 times, most recently from 55a9c37 to 8cf7eab Compare April 1, 2024 22:02
@boorad boorad force-pushed the feat/heifner-version branch from 8cf7eab to 4aa9968 Compare April 2, 2024 00:36
@boorad
Copy link
Collaborator Author

boorad commented Apr 2, 2024

@craftzdog this is ready for your review. If you approve, please merge and cut a v2.1.0 release.

FYI, I found act to run GHA locally and not spam PRs with tons of commits 👼

 act -j build_android_example --remote-name b

@boorad boorad marked this pull request as ready for review April 2, 2024 00:47
@craftzdog craftzdog merged commit 1ca3ba9 into craftzdog:master Apr 2, 2024
2 checks passed
@craftzdog
Copy link
Owner

Congrats!! Thanks so much for your effort, @boorad 🥳

@boorad
Copy link
Collaborator Author

boorad commented Apr 2, 2024

Happy to do it.

Can you cut a tag/release, and publish to npm? 🙏

@craftzdog
Copy link
Owner

@boorad
Copy link
Collaborator Author

boorad commented Apr 2, 2024

@boorad boorad deleted the feat/heifner-version branch April 2, 2024 14:34
@boorad
Copy link
Collaborator Author

boorad commented Apr 2, 2024

Also, the release and the tag for v2.1.0 are incorrect. They missed my commit. I'm going to try to fix.

For some reason in my react-native-quick-crypto PR, I am not getting a proper dependency download to node_modules. It gets package.json at "2.1.0", but android/CMakeLists.txt is the old one. Very weird. Maybe the npmjs.org publish will help this.

@craftzdog
Copy link
Owner

oh sorry, I didn't realize that it failed to publish due to:

npm notice Publishing to https://registry.npmjs.org/ with tag latest and default access
npm http fetch PUT 415 https://registry.npmjs.org/react-native-quick-base64 2086ms
npm ERR! code E415
npm ERR! 415 Unsupported Media Type - PUT https://registry.npmjs.org/react-native-quick-base64 - Hard link is not allowed

npm ERR! A complete log of this run can be found in: /Users/nora/.npm/_logs/2024-04-02T23_51_58_037Z-debug-0.log

I'll look into it soon

@craftzdog
Copy link
Owner

It's up. Can you try again?
https://www.npmjs.com/package/react-native-quick-base64/v/2.1.0

@boorad
Copy link
Collaborator Author

boorad commented Apr 3, 2024

💥 works like a charm 💥

Thanks!

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.

2 participants