Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

For #11753 - Update compose to 1.1.0 and Kotlin to 1.6.10 #11754

Merged
merged 2 commits into from
Feb 24, 2022
Merged

For #11753 - Update compose to 1.1.0 and Kotlin to 1.6.10 #11754

merged 2 commits into from
Feb 24, 2022

Conversation

Mugurell
Copy link
Contributor

@Mugurell Mugurell commented Feb 22, 2022 •
edited
Loading

This also required updating room to >= 2.4.0.
This new version adds a deprecation of the MigrationTestHelper api used in
LoginExceptionStorageTest that is to be later fixed in #11765.

activity_compose was also updated to the latest stable version to ensure a
better match with the latest stable version for compose.

Most of the changes are around supporting exhaustive whens for sealed classes as a new kotlin feature - https://kotlinlang.org/docs/whatsnew1530.html#exhaustive-when-statements-for-sealed-and-boolean-subjects

Updating kotlin-coroutines will be a followup - #11755

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

gabrielluong
gabrielluong reviewed Feb 22, 2022
View reviewed changes
...-collections/src/main/java/mozilla/components/feature/tab/collections/db/TabCollectionDao.kt
@@ -48,6 +48,7 @@ internal interface TabCollectionDao {
)
fun getTabCollections(): Flow<List<TabCollectionWithTabs>>

@Transaction
Copy link
Member

@gabrielluong gabrielluong Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Are we intentionally adding @Transaction for this query because the returned list could be fairly big and therefore want consistent result or for some other reason?

Copy link
Contributor

@jonalmeida jonalmeida Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is probably for consistency. The Room upgrade must have added a new lint rule/failure to ensure we were always adding the @Transaction annotation.

It probably came in from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, thanks Jonathan.
After updating to a new room version we'd get a warning here saying

The return value includes a POJO with a @relation. It is usually desired to annotate this method with @transaction to avoid possibility of inconsistent results between the POJO and its relations.

gabrielluong
gabrielluong reviewed Feb 22, 2022
View reviewed changes
Copy link
Member

@gabrielluong gabrielluong left a comment

Choose a reason for hiding this comment

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

Can we update the commit and title to include the kotlin version bump?

@jonalmeida
Copy link
Contributor

Interesting - there is #11516 which was doing corountines and kotlin 1.6 together. If this works, then let's land it. 🙂

@gabrielluong
Copy link
Member

Reopening this since the CI seems to be stuck in queued.

@gabrielluong gabrielluong reopened this Feb 22, 2022
@rvandermeulen
Copy link
Contributor

@gabrielluong CI isn't stuck, the gradle dependencies job is failing.
https://firefox-ci-tc.services.mozilla.com/tasks/fbEZS840QPGL6iXjKxYiFg/runs/5/logs/public/logs/live.log

@rvandermeulen
Copy link
Contributor

Looks like version 1.5.0 of activity-compose is still in alpha and 1.4.0 is the latest stable release.

This was referenced Feb 23, 2022
Closed
Closed
@Mugurell Mugurell changed the title For #11753 - Update compose to 1.1.0 For #11753 - Update compose to 1.1.0 and Kotlin to 1.6.10 Feb 23, 2022
@Mugurell
For mozilla-mobile#11753 - Update compose to 1.1.0 and Kotlin to 1.6.10
b2b1bc8 
This also required updating room to >= 2.4.0.
This new version adds a deprecation of the `MigrationTestHelper` api used in
`LoginExceptionStorageTest` that is to be later fixed in mozilla-mobile#11765.

activity_compose was also update to the latest stable version to ensure a
better match with the latest stable version for compose.
@Mugurell
Copy link
Contributor Author 

executing ['git', 'clone', 'https://github.com/mozilla-mobile/android-components', '/builds/worker/checkouts/src']
Cloning into '/builds/worker/checkouts/src'...
error: RPC failed; curl 56 GnuTLS recv error (-54): Error in the pull function.

Restarting CI.

@Mugurell Mugurell closed this Feb 23, 2022
@Mugurell Mugurell reopened this Feb 23, 2022
@Mugurell Mugurell added the 🛬 needs landing PRs that are ready to land label Feb 24, 2022
@mergify mergify bot merged commit e9cbe11 into mozilla-mobile:main Feb 24, 2022
@gabrielluong gabrielluong mentioned this pull request Feb 24, 2022
Closed
@Mugurell Mugurell mentioned this pull request Feb 28, 2022
Merged
@Mugurell Mugurell deleted the updateCompose branch May 17, 2022 16:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gabrielluong gabrielluong gabrielluong approved these changes

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

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

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

@grigoryk grigoryk Awaiting requested review from grigoryk

Assignees
No one assigned
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mugurell @jonalmeida @gabrielluong @rvandermeulen