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.

Sign up for GitHub

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

Jump to bottom

Adding React Native Themis #902

Merged
merged 8 commits into from
Mar 5, 2022
Merged

Adding React Native Themis #902

merged 8 commits into from
Mar 5, 2022

Conversation

radetsky
Copy link
Contributor

@radetsky radetsky commented Mar 4, 2022

Checklist

  • Change is covered by automated tests
  • Benchmark results are attached (if applicable)
  • The coding guidelines are followed
  • Public API has proper documentation
  • Example projects and code samples are up-to-date (in case of API changes)
  • Changelog is updated (in case of notable or breaking changes)

ilammy
ilammy approved these changes Mar 5, 2022
View reviewed changes
Copy link
Collaborator

@ilammy ilammy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't be looking at the code too deeply since it's kind of already released 😅 (And I've looked at it already in the different repo during development). Frankly, the amount of boilerplate here is staggering, but... ¯\_(ツ)_/¯

I have to say, this is an amazing piece of work!

Should be added in this PR:

  • Get a note about React Native support into the changelog.

Would be nice to get, but maybe not in this PR if you're not in the mood:

  • Teach CI to verify React Native build and packaging
  • Teach CI to verify React Native unit tests
  • Teach CI to verify React Native examples

Open question:

  • Integrate React Native wrapper with top-level build?

    That is, get the top-level build.gradle to be able to build RN stuff. And make that Xcode project in src/wrappers a part of Themis.xcodeproj at the top-level too.

    I'm not sure we'd want this, really, but Android Themis and Java Themis do coexist at the top-level, so maybe new additions should follow suit.

@vixentael vixentael changed the title React native themis Adding React Native Themis Mar 5, 2022
vixentael
vixentael reviewed Mar 5, 2022
View reviewed changes
docs/examples/react-native/android/app/build_defs.bzl
Comment on lines +3 to +18
def create_aar_targets(aarfiles):
for aarfile in aarfiles:
name = "aars__" + aarfile[aarfile.rindex("/") + 1:aarfile.rindex(".aar")]
lib_deps.append(":" + name)
android_prebuilt_aar(
name = name,
aar = aarfile,
)

def create_jar_targets(jarfiles):
for jarfile in jarfiles:
name = "jars__" + jarfile[jarfile.rindex("/") + 1:jarfile.rindex(".jar")]
lib_deps.append(":" + name)
prebuilt_jar(
name = name,
binary_jar = jarfile,
Copy link
Contributor

Choose a reason for hiding this comment

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

omg i hope we understand what's happening here

src/wrappers/themis/react-native-themis/react-native-themis.podspec Outdated Show resolved Hide resolved
src/wrappers/themis/react-native-themis/package.json Outdated Show resolved Hide resolved
...ers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java Outdated Show resolved Hide resolved
...ers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java Show resolved Hide resolved
src/wrappers/themis/react-native-themis/ios/Themis.xcodeproj/project.pbxproj
"$(SRCROOT)/../../../React/**",
"$(SRCROOT)/../../react-native/React/**",
);
IPHONEOS_DEPLOYMENT_TARGET = 9.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IPHONEOS_DEPLOYMENT_TARGET = 9.0;
IPHONEOS_DEPLOYMENT_TARGET = 10.0;

i dunno, shall we change to 10.0 or even 12.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a default value for React-Native projects. Let me few minutes to make the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, if it's default, we can leave it as is

src/wrappers/themis/react-native-themis/ios/Themis.xcodeproj/project.pbxproj Show resolved Hide resolved
@vixentael
Copy link
Contributor

@ilammy thank you for your suggestions. right now we try to move with small PRs (this one is an exception), so we will update readme, add tests, etc, but as standalone pieces.

@radetsky plz take a look on my comments

@vixentael
Copy link
Contributor

ObjCThemis / Code examples (pull_request)

these tests are failing because of Carthage

@vixentael vixentael merged commit d5154b0 into master Mar 5, 2022
@vixentael vixentael added O-ReactNative ⚛️ ReactNative platform O-Android 🤖 Operating system: Android O-iOS 📱 Operating system: iOS labels Mar 5, 2022
radetsky added a commit that referenced this pull request Apr 5, 2022
@radetsky @vixentael
Adding React Native Themis (#902)
ab3a58a 
* added react-native support

* rename secureCell functions

* remove themis framework from source tree

* extremely new example for RNThemis

* added README.md for npm package

* Update src/wrappers/themis/react-native-themis/react-native-themis.podspec

Co-authored-by: vixentael <[email protected]>

* Update src/wrappers/themis/react-native-themis/package.json

Co-authored-by: vixentael <[email protected]>

* Update src/wrappers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java

Co-authored-by: vixentael <[email protected]>

Co-authored-by: vixentael <[email protected]>
vixentael added a commit that referenced this pull request Apr 6, 2022
@radetsky @vixentael @ilammy
Release of React Native Themis (#908)
2667a99 
* Adding React Native Themis (#902)

* added react-native support

* rename secureCell functions

* remove themis framework from source tree

* extremely new example for RNThemis

* added README.md for npm package

* Update src/wrappers/themis/react-native-themis/react-native-themis.podspec

Co-authored-by: vixentael <[email protected]>

* Update src/wrappers/themis/react-native-themis/package.json

Co-authored-by: vixentael <[email protected]>

* Update src/wrappers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java

Co-authored-by: vixentael <[email protected]>

Co-authored-by: vixentael <[email protected]>

* update readme, mention themis rn (#903)

* update README and example to use 0.14.3 version from npm registry (#905)

* Improve handling of optional context for Secure Cell Token Protect and Seal (#906)

* update README and example to use 0.14.3 version from npm registry

* add typescript types

* optional context, declarations file

* update CHANGELOG

* Update CHANGELOG.md

Co-authored-by: Alexei Lozovsky <[email protected]>

* Update CHANGELOG.md

Co-authored-by: vixentael <[email protected]>

Co-authored-by: vixentael <[email protected]>
Co-authored-by: Alexei Lozovsky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vixentael vixentael vixentael left review comments

@ilammy ilammy ilammy approved these changes

@julepka julepka Awaiting requested review from julepka julepka is a code owner

Assignees
No one assigned
Labels
O-Android 🤖 Operating system: Android O-iOS 📱 Operating system: iOS O-ReactNative ⚛️ ReactNative platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@radetsky @vixentael @ilammy