-
Notifications
You must be signed in to change notification settings - Fork 5
Upgrade kmp and java apps to use 5.0.0-preview.3 #186
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
base: main
Are you sure you want to change the base?
Conversation
e0d95f1
to
f133c2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Upgrades Kotlin Multiplatform and Java applications to use Ditto SDK version 5.0.0-preview.3, updating the API implementation to match the new SDK interface.
- Updated Ditto SDK version from 5.0.0-preview.1 to 5.0.0-preview.3
- Migrated from DittoIdentity-based authentication to new authentication provider pattern
- Changed API method calls to use new naming conventions (e.g.,
registerObserver
toobserve
,getString
toasString
)
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
kotlin-multiplatform/composeApp/build.gradle.kts | Updated Ditto dependency versions to 5.0.0-preview.3 |
kotlin-multiplatform/composeApp/src/commonMain/kotlin/com/ditto/quickstart/ditto/DittoManager.kt | Refactored Ditto initialization to use new authentication pattern and updated API calls |
kotlin-multiplatform/composeApp/src//kotlin/com/ditto/quickstart/ditto/DittoManager..kt | Updated platform-specific Ditto creation to use DittoFactory |
kotlin-multiplatform/composeApp/src/commonMain/kotlin/com/ditto/quickstart/data/DittoCredentials.kt | Added new data class for Ditto credentials |
kotlin-multiplatform/composeApp/src/commonMain/kotlin/com/ditto/quickstart/di/DittoModule.kt | Updated dependency injection to include credentials |
kotlin-multiplatform/composeApp/src/commonMain/kotlin/com/ditto/quickstart/data/repository/DittoTaskRepository.kt | Updated API method call from registerObserver to observe |
kotlin-multiplatform/composeApp/src/commonMain/kotlin/com/ditto/quickstart/ui/TaskAddEditScreen.kt | Minor UI layout adjustment |
kotlin-multiplatform/README.md | Updated API reference link to preview.3 version |
java-spring/src/main/java/com/ditto/example/spring/quickstart/service/DittoService.java | Migrated to new authentication pattern and updated transport configuration |
java-spring/src/main/java/com/ditto/example/spring/quickstart/service/DittoTaskService.java | Updated API method calls to use new naming conventions |
java-spring/build.gradle.kts | Updated Ditto dependency versions to 5.0.0-preview.3 |
java-spring/java-spring-maven/pom.xml | Added Maven configuration with preview.3 dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
kotlin-multiplatform/composeApp/src/commonMain/kotlin/com/ditto/quickstart/di/DittoModule.kt
Outdated
Show resolved
Hide resolved
f133c2f
to
f7377a2
Compare
4735823
to
988c595
Compare
resolves issue on iOS where a new task couldn’t be added because the button is hidden behind the keyboard
988c595
to
1990e36
Compare
1990e36
to
b9055f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the expected status of tests? The browserstack CI failure also happens for me locally, and I have similar failures in both the Android and iOS integration tests which are not being run in CI.
Otherwise, mostly nits here.
@Nonnull | ||
public Flux<List<Task>> observeAll() { | ||
final String selectQuery = "SELECT * FROM %s WHERE NOT deleted ORDER BY title ASC".formatted(TASKS_COLLECTION_NAME); | ||
final String selectQuery = "SELECT * FROM %s WHERE NOT deleted".formatted(TASKS_COLLECTION_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason to drop the ORDER BY
here? The equivalent in the KMP quickstart retains it.
|
||
- [Kotlin Multiplatform Roadmap and Support Policy](https://docs.ditto.live/sdk/latest/install-guides/kotlin/multiplatform-roadmap) | ||
- [API Reference](https://software.ditto.live/java/ditto-java/5.0.0-preview.1/api-reference/) | ||
- [API Reference](https://software.ditto.live/java/ditto-java/5.0.0-preview.3/api-reference/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new API reference doesn't appear to be deployed yet
private val scope = CoroutineScope(SupervisorJob()) | ||
val secrets: DittoSecretsConfiguration | ||
|
||
constructor(secrets: DittoSecretsConfiguration) { | ||
this.secrets = secrets | ||
this.scope = CoroutineScope(SupervisorJob()) | ||
} | ||
|
||
private val scope: CoroutineScope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this change isn't really necessary, but it all works fine.
I don't see any reason the scope needs to move inside a constructor. For the DittoSecretsConfiguration
, you could just as easily statically reference it in-line, though storing it as a property protects against some potential refactors.
// Cloud sync is intentionally disabled to avoid authentication issues in test environments. | ||
// When enabled, Ditto Cloud Sync requires additional auth setup that causes certificate | ||
// validation failures in BrowserStack. Disabling ensures sync occurs via WebSocket URLs only. | ||
enableDittoCloudSync = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This configuration is no longer happening explicitly. Is there any reason we might still want to hold onto some version of the comment? (I lean toward no, but want to ask the question anyway)
).apply { | ||
auth?.setExpirationHandler { ditto, secondsRemaining -> | ||
// Authenticate when a token is expiring | ||
val clientInfo = ditto.auth?.login( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clientInfo
is unused here
provider = DittoAuthenticationProvider.development(), | ||
) | ||
} | ||
}.apply { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: All this stuff could live in a single apply
block instead of two
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving to approve after in-person convo that we haven't been maintaining those tests. We should probably update them at some point but I don't want to gatekeep this PR on that.
Preview 3 is not yet public. It is accessible to authorized users via the maven central staging APIs.
java-spring/
tojava-server/
java-server/java-spring-maven/
subproject for testing Maven compatibility