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

Add Properties class for Swift wrappers #1138

Merged
merged 10 commits into from
May 2, 2023
Merged

Add Properties class for Swift wrappers #1138

merged 10 commits into from
May 2, 2023

Conversation

absaroj
Copy link
Contributor

@absaroj absaroj commented Apr 24, 2023

Swift Wrappers:
Adding Swift wrappers to 1DSSDK. Since C++ code can only be accessed from Swift via Obj-C or C, I am taking Obj-C with bridging header.

Wrapping ODWEventProperties.mm class for Swift. Contains other supporting files:

  • local build script to build swift wrapper files only
  • cmake list file to generate the xcode project files
  • bridging header to access obj-c code from swift
  • main swift file to check the call to added wrapper

@absaroj absaroj changed the title Add build script and cmake file to build Swift wrappers (only) Add Properties class for Swift wrappers Apr 28, 2023
@@ -0,0 +1,25 @@
CMAKE_MINIMUM_REQUIRED(VERSION 3.16)
Copy link
Contributor

Choose a reason for hiding this comment

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

This version is higher than root level and Obj-C wrapper level versions, why?

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 was done when swift generators were failing to be generated and there was one suggestion to change it to this version.
But i think with xcode generators, this is not required. i can change it back to similar to obj-c.

There is below warning of deprecation though:

_CMake Deprecation Warning at CMakeLists.txt:1 (CMAKE_MINIMUM_REQUIRED):
Compatibility with CMake < 2.8.12 will be removed from a future version of
CMake.

Update the VERSION argument value or use a ... suffix to tell
CMake that the project does not need compatibility with older versions._


let props = EventProperties(name:"TestEvent")
props.setProperty("PropName", withValue: ["Type":"SwiftWrappers"])
props.setProperty("PropWithPII", withInt64Value: Int64(30), withPiiKind: ODWPiiKind.distinguishedName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are consumers going to use Obj-C constants/enums, or are you going to setup new ones for Swift?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am intending to let consumers use the obj-c enums instead of creating new ones in swift

wrappers/swift/CMakeLists.txt Show resolved Hide resolved
wrappers/swift/CMakeLists.txt Show resolved Hide resolved
- value: A Date that contains the property value.
- piiKind: The kind of Personal Identifieable Information (PII), one from the ::ODWPiiKind enum values.
*/
public func setProperty(_ name: String, withDateValue value: Date, withPiiKind piiKind: ODWPiiKind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are using the obj-c definitions ( e.g. ODWPiiKind ) in swift code, are there any interoperability issues between obj-c and swift that the app developer be aware of - E.g. interoperability across Swift compiler version and Obj-c runtime version etc. And as I understand, we can't avoid obj-c usage eventually, as the C++ code can't be directly called from swift ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the point.
Yes, we have to access c++ via objc from swift.

Though I haven't noticed any interoperability issues while building and running the code. However there are minor changes when it comes to using the enum declared in objc from swift, such as Enum prefix name is removed while using. But since they are minor, i thought of keep using the objc enums rather than creating their duplicate in swift.
also, i have create a task to investigate more about the different compiler and version compatibility between swift-objc and update the source files accordingly.

wrappers/swift/SDWEventProperties.swift Show resolved Hide resolved
@absaroj absaroj requested a review from lalitb May 1, 2023 22:46
Copy link
Contributor

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Looks Good, with some nit comments. Thanks for the PR.

set(PLATFORM_FILES main.swift SDWEventProperties.swift)
set(CMAKE_Swift_FLAGS "${CMAKE_Swift_FLAGS} -import-objc-header Swift_Wrapper-Bridging-Header.h")
add_executable(swift_sample ${PLATFORM_FILES})
target_link_libraries(swift_sample)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't add any extra property, is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you asking about the last line? it i used to create the final exe by linking all the libs.

@absaroj absaroj merged commit 0f06e94 into main May 2, 2023
@absaroj absaroj deleted the absaroj/add_a_class branch May 2, 2023 05:01
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