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(182): Started to plug the iOS business logic on the KMP module #183

Merged
merged 7 commits into from
Mar 1, 2024

Conversation

enthuan
Copy link
Collaborator

@enthuan enthuan commented Mar 1, 2024

OK, this is a big PR again, sorry for that :(

I set up and plugged the KMP module on iOS 🥳

And the good news is that it works fine! BUT I had to do some dirty hacks that will hopefully be temporary.
The biggest one is that Swift can't handle the Kotlin Result class; so I had to add a dependency on a wrapper to be able to use the results without breaking everything. You will notice I commented the .recover methods as the wrapper doesn't implement it.

Some constants were extracted from the shared module to be able to customize them depending on the platform, but I'm not sure it will remain like that, because I liked the fact that the app module doesn't know the URLs of the service for example.

I also noticed differences between Android and iOS, specifically on the Firebase authentication part. iOS doesn't use any token for the moment. Should we add this feature?

The Partner classes were also merged between Android and iOS (on Android the PartnerGroup was a Partner and the Partner was a Logo ; I just renamed the classes to match the iOS names)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, I can manage to get rid of this one :/

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

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

Noice 🚀

The biggest one is that Swift can't handle the Kotlin Result class;

Sounds like a good candidate for SKIE

I also noticed differences between Android and iOS, specifically on the Firebase authentication part. iOS doesn't use any token for the moment. Should we add this feature?

Do you mean iOS doesn't have Google signin? We could add it but TBH the GCP setup is a pain. It's great if we can add it but I would probably leave it out for the moment. Plus we're taking the risk of Apple asking for Apple signin, etc...

@enthuan
Copy link
Collaborator Author

enthuan commented Mar 1, 2024

Sounds like a good candidate for SKIE

Oh yeah, thank you for the idea, I upvoted the discussion on their GitHub

Do you mean iOS doesn't have Google signin? We could add it but TBH the GCP setup is a pain. It's great if we can add it but I would probably leave it out for the moment. Plus we're taking the risk of Apple asking for Apple signin, etc...

Yes I only see references to crashlytics on iOS (and I even don't have the Google-Service.json for the iOS application). The GraphQL interceptor only add the header for the "conference" and not any Authorization.

I don't really know on Android how GraphQL manages the bookmarks with this token 🤷‍♂️ Do you have a base with all the users token and their bookmarks ?

@martinbonnin
Copy link
Contributor

Oh yeah, thank you for the idea, I upvoted the discussion on their GitHub

Nice! Do you have the link to the discussion? I'll upvote too.

I don't really know on Android how GraphQL manages the bookmarks with this token 🤷‍♂️ Do you have a base with all the users token and their bookmarks ?

Exactly! It's in google Datastore. The corresponding code is here

@enthuan
Copy link
Collaborator Author

enthuan commented Mar 1, 2024

Nice! Do you have the link to the discussion? I'll upvote too.

touchlab/SKIE#47
It's related to inline classes such as Result.

@enthuan enthuan merged commit 4b72848 into paug:main Mar 1, 2024
1 check passed
@enthuan enthuan deleted the feat/182 branch March 1, 2024 10:43
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