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

Output android translations as Kotlin Multiplatform project #640

Merged
merged 13 commits into from
Jun 18, 2023

Conversation

sleeyax
Copy link
Member

@sleeyax sleeyax commented Jun 10, 2023

Migrated from the regular android project output to a multiplatform project. Basically, android/main now becomes android/androidMain and android/commonMain has been added in order to deliver common translations (in kotlin class format) to other multiplatform projects.

This PR is not intended as a breaking change. Nothing should change by this PR besides added support for Kotlin Multiplatform or KMM projects that wish to add translations to their commonMain sourceset.

Marked as draft because I need to run more tests and adjust the publishing configuration.

Migrated from the regular android project output to a multiplatform project. Bascially, `android/main` now becomes `android/androidMain` and `android/commonMain` has been added in order to deliver common translations (in kotlin class format) to other multiplatform projects.
This should build both AAR and multiplatform artifacts.
@sleeyax sleeyax requested a review from TheBeastLT June 11, 2023 21:26
scripts/build-kmm.js Outdated Show resolved Hide resolved
Copy link
Member Author

@sleeyax sleeyax left a comment

Choose a reason for hiding this comment

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

Curious, is there a particular reason to use node 12? (which is EOL since 30 April 2022 btw)

@TheBeastLT
Copy link
Contributor

Curious, is there a particular reason to use node 12? (which is EOL since 30 April 2022 btw)

For build scripts probably there's no particular reason, but this version is used when running streaming server on android.

@sleeyax
Copy link
Member Author

sleeyax commented Jun 16, 2023

Curious, is there a particular reason to use node 12? (which is EOL since 30 April 2022 btw)

For build scripts probably there's no particular reason, but this version is used when running streaming server on android.

But doesn't this repo only contain build scripts and no code that's ever to be executed by the streaming server? If so, there's no particular reason to stick to Node 12. I'll stick to it for now for consistency reasons, but perhaps we should consider upgrading to a newer version (in a new PR) so we can benefit from new language features that would make the code more readable and easier to maintain.

This improves reusability accross different scripts.
They can be internal when translations are included as a sub-package but since this is a public package that's being intalled as a dependency they should be public.
It's not really needed.
Without KSP, these need to be registered manually. The DX is still good by exposing these within the package.
Contrucors and methods have a field limit of 255, according to the JVM spec. Therefore we can't use regular data classes because constructing them results in going far above this limit unfortunately. An interface to implement works better for our use-case.
@sleeyax sleeyax marked this pull request as ready for review June 16, 2023 19:53
@sleeyax
Copy link
Member Author

sleeyax commented Jun 16, 2023

I've marked this PR as ready. Did some profiling and the implementation with lyricist is stable in a KMM (android) app, just about the same as a custom implementation with strings.xml. Suggestions or additional feedback are welcome.

@jaruba
Copy link
Member

jaruba commented Jun 17, 2023

@sleeyax

I'll stick to it for now for consistency reasons

this is pretty much the reason, everyone sticks to their tasks and we only upgrade things when necessary as we have way too many repos to maintain

@sleeyax sleeyax merged commit bd99774 into master Jun 18, 2023
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.

3 participants