[shared_preferences] Platform implementations of async api #6965

tarrinneal · 2024-06-21T04:23:28Z

part 2 of #5210

tarrinneal · 2024-07-19T03:30:43Z

@reidbaker any chance you can help me diagnose this gradle issue?

stuartmorgan · 2024-07-19T17:14:32Z

I believe that ("Unsupported class file major version") is the result of something requiring a newer Java version than is available on the machine?

reidbaker · 2024-07-19T17:20:38Z

Unsupported class file major version 65 means you are using java 21 and should be using something lower more detail here

Looking at your changes I dont see anything that should cause this failure. @christopherfujino do you see something about this machine config that would cause it to run java 21 instead of the lower java versions we expect.

stuartmorgan · 2024-07-19T17:22:35Z

The machine should definitely be using 17:

packages/.ci.yaml

Line 32 in 3144ac0

{"dependency": "open_jdk", "version": "version:17"},

Could it be something precompiled that used 21?

packages/shared_preferences/shared_preferences_android/android/build.gradle

...android/example/android/app/src/androidTest/java/io/flutter/plugins/DartIntegrationTest.java

...d_preferences_foundation/Sources/shared_preferences_foundation/SharedPreferencesPlugin.swift

...ages/shared_preferences/shared_preferences_foundation/lib/shared_preferences_foundation.dart

packages/shared_preferences/shared_preferences_android/lib/shared_preferences_android.dart

stuartmorgan · 2024-07-25T17:51:55Z

packages/shared_preferences/shared_preferences_android/android/build.gradle

@@ -2,13 +2,15 @@ group 'io.flutter.plugins.sharedpreferences'
 version '1.0-SNAPSHOT'

 buildscript {
+    ext.kotlin_version = '1.8.10'


@reidbaker Is this going to cause issues for clients? It looks like this would be the first of our plugins using 1.8.x.

@tarrinneal What exactly was the error when this was 1.7.10 like our other plugins?

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8741408197539124465/+/u/Run_package_tests/native_unit_tests/stdout?format=raw

Integration tests run fine though, only unit tests fail. There may be some other underlying issue.

packages/shared_preferences/shared_preferences_foundation/CHANGELOG.md

stuartmorgan · 2024-07-26T17:29:56Z

@tarrinneal I played around with a bit locally, and if I add the implementation(platform("org.jetbrains.kotlin:kotlin-bom:1.8.22")) workaround to android/build.gradle, drive-examples works (which suggests to me that clients should be fine), then if I add the kotlin_version bump to 1.8.10 only in example/android/build.gradle (which doesn't affect clients), unit tests compiled and passed.

@reidbaker Does that seem like a reasonable thing to do? I'm still not clear why this example needs a different kotlin_version when none of the others do, but maybe it's because the unit tests are Kotlin?

reidbaker · 2024-07-26T17:50:47Z

@tarrinneal I played around with a bit locally, and if I add the implementation(platform("org.jetbrains.kotlin:kotlin-bom:1.8.22")) workaround to android/build.gradle, drive-examples works (which suggests to me that clients should be fine), then if I add the kotlin_version bump to 1.8.10 only in example/android/build.gradle (which doesn't affect clients), unit tests compiled and passed.

@reidbaker Does that seem like a reasonable thing to do? I'm still not clear why this example needs a different kotlin_version when none of the others do, but maybe it's because the unit tests are Kotlin?

It seems weird to me that example/android/build.gradle cant use kotlin 1.8.22. The only reason for that would be if the dependencies used in example/android/app/build.gradle were too old to use a newer version of kotlin.

In general I think it is ok to have the example app have whatever version of kotlin but it seems to me like they should be aligned.

stuartmorgan · 2024-07-26T18:38:00Z

It seems weird to me that example/android/build.gradle cant use kotlin 1.8.22.

To clarify, maybe it can use 1.8.22. What doesn't work is continuing to use 1.7.10, even though that's what every other package we have that uses the bom workaround thing is doing.

stuartmorgan · 2024-07-26T18:43:04Z

And to put some numbers to that:

7 of our plugins currently use the implementation(platform("org.jetbrains.kotlin:kotlin-bom:1.8.22")) trick, with either 1.8.10 or 1.8.22
0 of our plugins (other than a Pigeon internal test app) set ext.kotlin_version to something newer than 1.7.10.

So in seven other plugins, just adding the bom line seems to be enough, but here we also need to bump the example app's Kotlin version.

reidbaker · 2024-07-26T20:11:01Z

IIRC there was an issue fixed in kotlin 1.8.* that fixed situations where when you had multiple kotlin versions between the app and libraries that you would have duplicate files and the work around was to add the kotlin bom.

@gmackall might remember the exact versions.

tarrinneal · 2024-07-27T01:21:30Z

Any ideas why the bom isn't working for this pr?

stuartmorgan · 2024-07-27T01:31:13Z

Since it works if we update Kotlin only in the example app, I would just add that to unblock the PR. We have the build-all app ensuring that it works in an app that doesn't set a higher Kotlin version.

stuartmorgan

LGTM!

stuartmorgan · 2024-07-30T16:57:22Z

packages/shared_preferences/shared_preferences_android/android/build.gradle

-    dependencies {
-        // org.jetbrains.kotlin:kotlin-bom artifact purpose is to align kotlin stdlib and related code versions.
-        // See: https://youtrack.jetbrains.com/issue/KT-55297/kotlin-stdlib-should-declare-constraints-on-kotlin-stdlib-jdk8-and-kotlin-stdlib-jdk7
-        implementation(platform("org.jetbrains.kotlin:kotlin-bom:1.8.10"))


Weird, I figured you'd still need this part. 🤷🏻

reidbaker · 2024-07-30T17:34:43Z

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

Are you stuck on legacy as the prefix? The meaning of legacy can change over time so v1 might be a better prefix.

I think legacy is pretty fitting, as it is now legacy code. It implies future intent to deprecate the code

The issue is legacy assumes 2 states, Legacy and Current and does not account for a 3rd state to exist at the same time.

This is something I don't remember to think about until you raise it in reviews, but I'm trying to internalize it into my normal review process :)

I agree it would have been better not to use that name, but in this case I think it'll end up okay, because this was a big change to an API surface that we very explicitly don't want a lot of churn to, and hasn't been fundamentally changed like this in the entire life of the plugin. It's a very different case than, e.g., Android older API version codepaths, where this has come up before, where we can easily get new ones every year.

Yep I did not view it as a blocker which was why it came paired with an approval. It was just the dismissal that "I think legacy is pretty fitting, as it is now legacy code." that I wanted to expand upon since that misses my entire reason for commenting on the name.

I should have linked to the style guide. https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-newold-modifiers-in-code

This code, and the names specifically, can also change any time without breaking anything, as this is just the native platform implementation. If there does end up being another massive overhaul before it's deprecated, we can just change it to something else at that time.

There's always LegacyLegacy ;)

yes there is hahaha 😭

It was just the dismissal that "I think legacy is pretty fitting, as it is now legacy code." that I wanted to expand upon since that misses my entire reason for commenting on the name.

Totally fair, I was missing the expanded context so I appreciate the comment :)

…lutter/packages#6965)

flutter/packages@99e8606...46a712f 2024-07-31 [email protected] [interactive_media_ads] Adds initial iOS implementation (flutter/packages#7063) 2024-07-31 49699333+dependabot[bot]@users.noreply.github.com [webview]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.1 in /packages/webview_flutter/webview_flutter_android/android (flutter/packages#6770) 2024-07-31 49699333+dependabot[bot]@users.noreply.github.com [interactive_media_ads]: Bump androidx.annotation:annotation from 1.8.0 to 1.8.1 in /packages/interactive_media_ads/android (flutter/packages#7238) 2024-07-30 [email protected] [shared_preferences] full api redesign with DataStore and cache-less interface (flutter/packages#5210) 2024-07-30 49699333+dependabot[bot]@users.noreply.github.com [url_launcher]: Bump androidx.browser:browser from 1.5.0 to 1.8.0 in /packages/url_launcher/url_launcher_android/android (flutter/packages#6811) 2024-07-30 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump com.android.billingclient:billing from 6.1.0 to 6.2.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#6413) 2024-07-30 [email protected] [path_provider] fix 151823: update minimum required path_provider_android to 2.2.0 (flutter/packages#7181) 2024-07-30 [email protected] [shared_preferences] Platform implementations of async api (flutter/packages#6965) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added platform-android platform-ios platform-linux platform-macos platform-web platform-windows p: shared_preferences labels Jun 21, 2024

tarrinneal force-pushed the spr-plat branch 4 times, most recently from 0a4a7f8 to 70bb580 Compare June 28, 2024 05:53

plat

0b619b4

tarrinneal force-pushed the spr-plat branch from 70bb580 to 0b619b4 Compare July 18, 2024 21:02

tarrinneal marked this pull request as ready for review July 18, 2024 21:02

tarrinneal requested review from cbracken, reidbaker and ditman as code owners July 18, 2024 21:02

analyze

b8b4643

tarrinneal requested a review from stuartmorgan July 18, 2024 21:28

tarrinneal added 3 commits July 18, 2024 15:33

newer version

3216359

revert gradle changes

351e189

revert gradle wrapper

167a191

stuartmorgan requested changes Jul 19, 2024

View reviewed changes

tarrinneal added 3 commits July 19, 2024 12:41

gradle

751b7ab

bom

966cc82

move bom

f5a7965

tarrinneal added 4 commits July 22, 2024 17:08

one more legacy

48a294f

anotha one

9d4ce28

fix tests

2b7b3a6

revert test fix, fix real issue

fa7389d

tarrinneal requested a review from stuartmorgan July 23, 2024 04:30

tarrinneal added 2 commits July 24, 2024 14:59

fix explode

bc85c30

format

9780d68

stuartmorgan reviewed Jul 25, 2024

View reviewed changes

revert kotlin version, update changelog

9249648

tarrinneal added 3 commits July 26, 2024 14:48

bom

475237f

Merge branch 'main' of github.com:flutter/packages into spr-plat

94498a0

move bom

78ace3a

kotlin version, how do they work?

89ed80a

tarrinneal requested a review from stuartmorgan July 27, 2024 02:44

stuartmorgan approved these changes Jul 30, 2024

View reviewed changes

reidbaker approved these changes Jul 30, 2024

View reviewed changes

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 30, 2024

auto-submit bot merged commit 315ed8a into flutter:main Jul 30, 2024
76 checks passed

engine-flutter-autoroll mentioned this pull request Jul 31, 2024

Roll Packages from 99e860679a9a to 46a712f38301 (8 revisions) flutter/flutter#152622

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 31, 2024

315ed8a [shared_preferences] Platform implementations of async api (f…

0f8fa84

…lutter/packages#6965)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[shared_preferences] Platform implementations of async api #6965

[shared_preferences] Platform implementations of async api #6965

tarrinneal commented Jun 21, 2024 •

edited

Loading

tarrinneal commented Jul 19, 2024

stuartmorgan commented Jul 19, 2024

reidbaker commented Jul 19, 2024

stuartmorgan commented Jul 19, 2024

stuartmorgan Jul 25, 2024

tarrinneal Jul 25, 2024

stuartmorgan commented Jul 26, 2024

reidbaker commented Jul 26, 2024

stuartmorgan commented Jul 26, 2024 •

edited

Loading

stuartmorgan commented Jul 26, 2024

reidbaker commented Jul 26, 2024

tarrinneal commented Jul 27, 2024

stuartmorgan commented Jul 27, 2024

stuartmorgan left a comment

stuartmorgan Jul 30, 2024

reidbaker Jul 30, 2024

tarrinneal Jul 30, 2024

reidbaker Jul 30, 2024

stuartmorgan Jul 30, 2024

reidbaker Jul 30, 2024

tarrinneal Jul 30, 2024

tarrinneal Jul 30, 2024

reidbaker Jul 30, 2024

tarrinneal Jul 30, 2024

[shared_preferences] Platform implementations of async api #6965

[shared_preferences] Platform implementations of async api #6965

Conversation

tarrinneal commented Jun 21, 2024 • edited Loading

tarrinneal commented Jul 19, 2024

stuartmorgan commented Jul 19, 2024

reidbaker commented Jul 19, 2024

stuartmorgan commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Jul 26, 2024

reidbaker commented Jul 26, 2024

stuartmorgan commented Jul 26, 2024 • edited Loading

stuartmorgan commented Jul 26, 2024

reidbaker commented Jul 26, 2024

tarrinneal commented Jul 27, 2024

stuartmorgan commented Jul 27, 2024

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrinneal commented Jun 21, 2024 •

edited

Loading

stuartmorgan commented Jul 26, 2024 •

edited

Loading