Coroutine API redesign: dispatcher + injectable scope + injectable DataStore (2.0) - #10
Merged
Merged
Conversation
- Gradle 9.5.0 -> 9.6.1 (wrapper scripts regenerated) - Gradle plugin 9.2.1 -> 9.3.1 - Kotlin 2.4.0 -> 2.4.10 - Kover 0.9.8 -> 0.9.9 - lifecycle-runtime 2.10.0 -> 2.11.0 - Compose BOM 2026.05.01 -> 2026.06.01 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…aStore Implements COROUTINE-API-REDESIGN.md, taking the wider of the two options so the testability goal is actually delivered rather than deferred. One constructor parameter was doing two unrelated jobs: choosing the dispatcher for suspend functions AND owning fire-and-forget launches. Splitting them fixes three problems: - suspend functions no longer reparent onto a foreign Job, so clearPrefs() and the suspending writes are cancelled by their caller as they should be - the default SupervisorJob is now per-instance; the process-wide companion vals on both classes are deleted - consumers can inject a scope, so async write failures can reach a CoroutineExceptionHandler instead of the default handler BaseDataStoreHelper now has two constructors. The primary takes the DataStore directly, which is what makes subclasses unit-testable; the convenience constructor keeps the old (context, preferenceName) signature and builds the store via PreferenceDataStoreFactory. Existing subclasses compile unchanged — the 1,621-line existing suite passes untouched. readValueBlocking drops its runBlocking context: DataStore is already main-safe, so the IO hop bought nothing and the Job it carried made the read cancellable process-wide by accident. Adds BaseDataStoreHelperInjectionTest covering the injected DataStore, the injected scope, per-instance scope isolation, and caller-cancellable clearPrefs on both helpers. The two cancellation tests were verified against a mutated build: both fail under the old foreign-Job behaviour. BREAKING CHANGE: coroutineContext -> dispatcher (+ optional scope), and the companion supervisorJob properties are gone. See "Migrating to 2.0" in the README — in particular, any logout-style sequence that relied on prefs writes surviving its caller's cancellation now needs an application-lifetime scope or withContext(NonCancellable). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four protected boolean helpers on BaseDataStoreHelper have carried @deprecated annotations and a "remove in a future release" TODO for several versions. A major bump is the release to do it in. writeBool(key, Boolean) -> writeBoolean(key, value) writeBool(key, Boolean?) -> writeBoolean(key, value) readNullableBool(key) -> readBoolean(key) readBool(key) -> readBoolean(key, true) Nothing in this repo referenced them. Documented in the README migration note, including the trap in the last one: readBool defaulted to true, and readBoolean(key) alone is the nullable overload, so a mechanical replace-with that drops the argument changes behaviour silently. Also fixes the documentation command in CLAUDE.md. Dokka 2.2.0 removed the V1 dokkaHtml task, so the documented command has been failing since the Dokka upgrade; dokkaGenerateHtml is the replacement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The proposal has shipped and its content now lives where consumers will actually look for it: the migration guide in the README and the class KDoc on both helpers. The design record isn't lost — the doc, including the Outcome section recording where the implementation deviated from the proposal and why, is retrievable from history: git show 5c4f1dd:COROUTINE-API-REDESIGN.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Fix a misleading line: it said subclasses were fine if they didn't "pass a context", but BaseDataStoreHelper(context, "name") does pass a context. The thing that matters is not passing a coroutine context. - Call out that item 2 (caller-cancellable suspend functions) is the only change that can alter behaviour without a compile error. The rest are removed or retyped symbols that fail the build immediately, so that's where a reader's attention should go. - Add the 2.0.0-beta01 coordinate for testing ahead of the release. - Note the injectable DataStore as a new capability, since a reader skimming only the migration guide would otherwise miss it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The file was copied from another project and had drifted from what this repo actually needs. - Drop `sdk install maven` / `mvn -v`. This is a pure Gradle project; the release build was downloading Maven 3.9.16 every time and never using it. - Bump the SDKMAN JDK from 17 to 21.0.12, matching .github/workflows/ci.yml. CI and the release build were running different JDKs, which meant a green CI was not evidence the release would build — JitPack was the only place a JDK-specific break could surface, and only at release time. - Add an explicit `install:` scoped to :PrefsHelper. JitPack's default runs `assemble`, which built the :app sample module as well — 96 of 186 tasks for something that is never published. Takes the build from ~1m53s to ~12s. GROUP and VERSION are JitPack-provided env vars and are passed through, or the artifact would publish as "unspecified". Only :PrefsHelper applies maven-publish, so it is still the single published module and the consumer coordinate is unchanged. Verified locally on Corretto 21.0.12 with JitPack's own argument form, and the command was read off the build log of the 2.0.0-beta01 release rather than guessed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pin the convenience constructor's DataStore to Dispatchers.IO ------------------------------------------------------------ The convenience constructor was building its store with `scope = CoroutineScope(dispatcher + SupervisorJob())`, so a caller-supplied dispatcher also owned DataStore's internal actor. That was a silent change from the old `preferencesDataStore` delegate, which always used its own IO scope — and it combined badly with readValueBlocking's plain runBlocking: a confined dispatcher (Main, single-threaded, a paused test dispatcher) would deadlock the store against its own blocking read. The coupling bought nothing. Passing a test dispatcher to the convenience constructor never made anything deterministic — that is precisely why the injectable primary constructor exists. Pinned to Dispatchers.IO, restoring pre-2.0 isolation exactly, and documented so it doesn't get "tidied" back. Fix the migration guide banner ------------------------------ It claimed only item 2 changes behaviour without a compile error. Item 5 (readValueBlocking dropping its dispatcher hop) is equally silent. Now names both, while still pointing at item 2 as the one that can corrupt state. Also ---- - Document that an injected scope does NOT own DataStore's internal actor, so a CoroutineExceptionHandler on it won't see DataStore's own failures. - Add write-cancellation tests. The 2.0 claim covers clearPrefs *and* the suspending writes, but only clearPrefs was tested. Mutation-checked: restoring the foreign Job on the nullable writeValue overload fails exactly the new guard. - README: use a non-precreated unique path in the DataStore test example, matching the injection test. The old `newFile` form does actually work (verified), but it depends on non-contractual DataStore internals and contradicted our own test. - CLAUDE.md: two test classes -> three, named. - jitpack.yml: correct the timing claim in the comment. The "~12s" was a warm local build; on JitPack wall time is dominated by the Gradle distribution download. The real, measured win is 158 tasks -> 31. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DataStore's produceFile is contractually required to yield the same file on every invocation. Both the injection test and the README example called a counter-incrementing helper from inside the lambda, so a second invocation would have handed DataStore a different path — a worse failure than the tempFolder.newFile form this replaced, which would at least have thrown. Current DataStore resolves produceFile once via a lazy, so neither form misbehaves today. This is about not depending on that, and about the README being a safe thing to copy. Also renames the region the write-cancellation tests live in; they had been added under "region 4 — clearPrefs() is caller-cancellable". Mutation check re-run after the change: restoring the foreign Job on either clearPrefs or the nullable writeValue still fails exactly its own guard, so the rework didn't weaken the tests. Both raised by Grok's re-review as optional nits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The DataStore usage section still described writes going to "the library's internal scope". That scope is injectable as of 2.0, so it now says so and links to the section explaining it. - Install section gains the two facts a consumer needs before adding the dependency: minSdk 21, and that datastore-preferences is an api dependency so DataStore<Preferences> is visible without declaring it. Plus a pointer to the migration guide, since 2.x is breaking. - Make the NonCancellable logout snippet valid Kotlin rather than a bare ellipsis inside braces, matching the comment style used in the other snippets. All three internal anchors verified to resolve against the heading slugs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BasePrefsHelper gains Double; BaseDataStoreHelper gains Date. Both were previously supported by only one of the two, which made the "supported types" story awkward to explain and forced consumers to pick a backend partly on type coverage. SharedPreferences has no double primitive, so BasePrefsHelper stores a Double as its raw IEEE-754 bit pattern via putLong, read back with Double.fromBits. This round-trips every value exactly, including the denormals and infinities a Float-based encoding would mangle. The one sharp edge is documented on setDouble: reading that key with getLong returns the bit pattern, not the number. BaseDataStoreHelper stores a Date as epoch millis in a longPreferencesKey and removes the key on null, matching how it already handles its other temporal types rather than importing BasePrefsHelper's -1L sentinel. That means DataStore can represent Date(-1L) distinctly from absent, where SharedPreferences cannot — documented rather than papered over. Both additions get the full surface: read/write, async write, Flow accessors and blocking reads on the DataStore side, plus non-null and nullable *Pref delegates on both. Tests: 183 -> 199, all passing, coverage gate holding. Date coverage includes the epoch and pre-epoch boundaries, which is exactly where a sentinel-based scheme would have failed. Docs: README "Supported types" now states the shared list plus a table of the storage differences that genuinely remain between the backends; same in CLAUDE.md. Also drops the beta coordinate from the README, since those builds won't outlive the merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Float and Set<String> were the real gaps: both are first-class on SharedPreferences AND DataStore, so their absence was an oversight rather than a design decision. ByteArray, Instant and Set<Enum> round out the set. Backend-specific encodings, all documented at the call site: - Float and Set<String> are native on both backends. - ByteArray is native on DataStore; Base64 (NO_WRAP) in a String on SharedPreferences, which has no binary type. Undecodable data logs and returns null instead of throwing. - Instant is epoch millis: null removes the key on DataStore, and uses the established -1L sentinel on SharedPreferences alongside the other temporal types. - Set<Enum> is a set of Enum.name on both. Names that no longer match a constant are dropped on read, so deleting an enum value doesn't break existing installs. Set<String> copies on both read and write for SharedPreferences. The platform documents getStringSet's result as one callers must not modify, and keeps a reference to the set it is handed on write — both directions are traps, so both are copied. Tested from both sides. enumSetPref uses the same thin-inline + @PublishedApi-internal split as enumPref, for the IllegalAccessError gotcha in CLAUDE.md. This is verified, not assumed: collapsing the split so the inline function returns the anonymous object directly makes exactly the three enumSetPref delegate tests fail with IllegalAccessError, and nothing else. Noted in CLAUDE.md that only a subclass-in-another-module delegate test catches this. New BasePrefsHelperRealPrefsTest covers what mocks structurally cannot: the Double bit encoding, Base64, and the Set<String> copies against a real Robolectric SharedPreferences. That also closes the gap flagged when Double was added — its round-trip was previously only proven by composition. Tests 199 -> 298, all passing, coverage gate holding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ning
Null now means the same thing everywhere: assigning null removes the key,
an absent key reads back as null. BasePrefsHelper's -1L temporal sentinel
is gone.
The sentinel was not just an inconsistency, it was a live defect. Epoch
day -1 is 1969-12-31, so a LocalDate of that day was silently unstorable
and read back as null. Same for Date/Instant at 1ms before the epoch.
LocalTime was unaffected in practice, since -1 was never in its valid
0..86399 range — but it would now throw rather than misread, so
getLocalTime treats out-of-range values as null.
This changes how existing data reads, which is the sharp edge. A key left
at -1L by 1.x is no longer "no value", it is 1969-12-31.
migrateLegacyTemporalSentinels(vararg keys) sweeps those keys once and is
safe to re-run; documented as migration item 8, and the "silent changes"
banner now lists it as the only one that touches stored data.
Also adds migrateIfNeeded(currentVersion) { from -> }, a small
schema-version stamp so migrations that are NOT safe to repeat ("stored
seconds are millis now") can be written directly. The subtle part is
bootstrapping: an unstamped file is a pre-versioning install if it holds
anything, or a fresh install if empty. Doing this now matters — once 2.0
ships without a stamp, 1.x and 2.0 installs would both lack the key and
need different treatment, so the ambiguity only gets worse with time.
Downgrades never rewind the stamp.
BaseDataStoreHelper deliberately gets no equivalent. DataStore's own
DataMigration already tracks whether it has run and applies atomically
before the first read, which beats stamping a version afterwards; the
convenience constructor now takes a migrations list instead. Noted in
CLAUDE.md so nobody adds a parallel scheme for symmetry's sake.
Tests 298 -> 311. The existing temporal tests encoded the old semantics
and were updated; two of them turned out to be asserting that -1L means
"not set", so they were rewritten rather than mechanically converted —
one is now a regression guard proving 1969-12-31 round-trips.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The getInstance() double-checked singleton on NormalDataStore was not vestigial — DataStore still permits only one live instance per file, and MainActivity built PrefsHelper in onCreate, so without it a rotation would have constructed a second store and thrown. It was hand-rolled DI. Registering the helper as a Koin `single` gives the identical guarantee with none of the code, so getInstance() is gone. NormalPrefs, DevicePrefs and NormalDataStore are now injected into PrefsHelper. That also lets the sample demonstrate the two things 2.0 added that had no runnable example: - An application-lifetime scope carrying a CoroutineExceptionHandler, injected into NormalDataStore. This is the whole point of making `scope` injectable: without a handler, a failed fire-and-forget write reaches the thread's default handler and takes the process down. - migrateIfNeeded, in NormalPrefs, sweeping the pre-2.0 -1L sentinel. Side benefit: MainActivity was passing itself as the Context into long-lived helpers. They now get the application context. The library remains DI-agnostic — Koin is a :app dependency only. New PrefsModuleTest resolves the graph for real, because reference code that only compiles isn't proven. Mutation-checked: changing the NormalDataStore registration to `factory` fails exactly testDataStoreHelperIsASingleton, so the test genuinely pins the rule it claims to. Robolectric instantiates the manifest Application for every test, which would call startKoin repeatedly and throw once the JVM is reused, so robolectric.properties points it at the stock Application. No test uses the sample's graph — they all declare their own subclasses. README gains a "One instance per DataStore file" section covering both paths: DI registration for containers, and a correct double-checked singleton with applicationContext for apps without one. Tests 311 -> 316. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getEnumSet and the enumSetPref delegate branched on names.isEmpty(), which conflates two different states: the key being absent, and the key holding an explicitly stored empty set. Only the first should yield the default. The consequence was that an empty enum set could not be stored at all whenever the default was non-empty — writing emptySet() and reading it back returned the default. It also broke the parity claimed a commit earlier, since BaseDataStoreHelper reads via `readStringSetValue(key) ?: default` and therefore distinguishes null from empty correctly. Both call sites now check contains(key) first, matching how every other nullable accessor on this class decides absence. The gap is a little embarrassing: BasePrefsHelperRealPrefsTest already had testEmptyStringSetIsDistinctFromAbsent for Set<String> and the same reasoning simply wasn't applied to Set<Enum>. There are now equivalents on both helpers, including one on the DataStore side purely as a parity guard so the two cannot drift again. Also corrects a KDoc on BaseDataStoreHelper.writeDate that still described BasePrefsHelper as writing a -1L sentinel — true until the previous commit removed it. Found by Grok in review. Tests 316 -> 318. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two findings from Grok's third review, both real. clearPrefs() cleared KEY_HELPER_VERSION along with user data (score 88). That is a data-corruption path the sample's own shape sets up: stamp at v2, logout calls clearPrefs, the helper is a DI singleton so init never re-runs, the app writes a preference, and the file is now non-empty and unstamped. The next cold start reads that as a pre-versioning install and re-runs every migration. Harmless for the idempotent sentinel sweep, corrupting for exactly the non-idempotent migrations migrateIfNeeded exists to protect. Worse, the README recommends clearPrefs on logout in the same guide that introduces versioned migrations, so a consumer following both pieces of advice would hit it. The stamp is schema metadata, not user state, so clearPrefs now preserves it. Editor.clear() is applied before puts in the same edit regardless of call order, so the restore is atomic rather than a second commit. Both the unit case and the full logout-then-relaunch sequence are covered, and both were confirmed failing before the fix. setEnum(null) wrote "" instead of removing the key (score 82). Reads coped, but contains(key) stayed true here and false on BaseDataStoreHelper — directly contradicting 2.0's "null means the same thing on both helpers" claim. Now removes the key. Docs corrected where they overclaimed: - CLAUDE.md still said temporal types "preserve the existing -1L sentinel" in one bullet while another said it was gone. - README claimed every type has both delegate overloads; it does not, and the two helpers differ. Replaced with the actual matrix. - Migration item 8 listed Instant among types 1.x wrote sentinels for. Instant is new in 2.0 and never had one. - Made "list every temporal key" explicit about why the sweep is per-key and what a forgotten key looks like, and resolved the contradictory advice about whether to leave the call in place. PrefsModuleTest's facade assertion was half-theatre — it proved PrefsHelper was a single, not that the container's instances were the ones injected. It now writes through the facade and reads from the separately-resolved sub-helper. Tests 318 -> 321. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The README promised consumers need no ProGuard rules, but nothing in the repo exercised R8 — the sample built with isMinifyEnabled = false, unit tests never run R8, and Robolectric runs unminified code. The claim was plausible and untested. Now: - The sample's release build is minified. `:app:assembleRelease` produces no missing_rules.txt, which is R8 stating nothing needed keeping. That alone is the build-time half of the claim. - app/src/androidTest/R8SurvivalTest.kt round-trips enum, Set<Enum>, Double, ByteArray, Set<String> and Date preferences, plus the migration machinery, against the minified APK on a real device. 7/7 passing, with the sample obfuscated to NormalPrefs -> wn, Theme -> sw, Feature -> hh while Theme.DARK still persisted and reloaded as the literal "DARK" — the property enum prefs actually depend on. Manual by design: `./gradlew :app:connectedAndroidTest -PminifiedTests`. Not in CI, which has no device. Without the flag the tests run against unminified debug and prove nothing, so the first test asks the runtime whether its own class was renamed and fails loudly if not, rather than inferring from the build type. The sample gained Theme/Feature enum preferences so the obfuscated code under test is production-shaped rather than test-only classes. Two sets of keep rules were needed, both harness-only and both kept out of proguard-rules.pro on purpose, since that file staying empty is the whole signal: - proguard-rules-androidtest.pro disables shrinking of the test APK. - proguard-rules-instrumentation.pro is applied to the app ONLY under -PminifiedTests. AGP does not duplicate classes already on the app's classpath into the test APK, so anything the test uses and the app does not gets stripped from both. Uses -keepclassmembers rather than -keep so classes are still renamed and the obfuscation guard stays honest. JVM suite unaffected: 321 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stion All four residual items from the re-review, none of which blocked. - R8SurvivalTest KDoc contradicted itself: said a debug run "will still pass" and also that the minify guard fails loudly. The guard does fail, so a debug run reports failure. Reworded. - Nullable enumPref KDoc still said assigning null "clears the stored value"; it removes the key as of 2.0. Corrected, with the pre-2.0 behaviour noted since that is what makes contains() differ. - Dropped a pointless companion-object init in R8SurvivalTest that called InstrumentationRegistry for no effect, and its now-unused import. The open question was whether android.r8.strictFullModeForKeepRules=false was propping up the clean R8 result. It is not: with it flipped to true, :app:assembleRelease still produces no missing_rules.txt and the on-device suite still passes 7/7 against the minified build. gradle.properties keeps its original value — changing a project-wide R8 setting is not this change's business — but the README now states the claim holds either way rather than hedging with "as configured". 321 JVM tests, 0 failures. Release build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakeeilbeck
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the coroutine API redesign proposal (the spec doc is removed in this branch now that it has shipped — readable at
git show 5c4f1dd:COROUTINE-API-REDESIGN.md), plus the Gradle/dependency bump and a deprecation clear-out that a major version is the right moment for.This is a breaking change and wants a major version tag.
Why
One constructor parameter was doing two unrelated jobs — choosing the dispatcher for
suspendfunctions and owning fire-and-forget launches. That conflation caused three problems:withContext(coroutineContext)reparented work onto a foreignJob, soclearPrefs()and the suspending writes behaved likeNonCancellable— asuspendfunction that its caller cannot cancel.SupervisorJobwas acompanion val: one Job shared by every helper instance in the process. Cancel it once and every prefs helper in the app silently stops doing async work, forever.What changed
dispatcher(noJob) decides wheresuspendfunctions work;scopeowns the*Asynclaunches and is injectable. The defaultscopeis now per-instance.BaseDataStoreHelpergains a second constructor. The primary takes theDataStore<Preferences>directly — that's what makes subclasses unit-testable; the convenience constructor keeps the old(context, preferenceName)signature and builds the store viaPreferenceDataStoreFactory.Also:
readValueBlockingdrops itsrunBlockingcontext (DataStore is already main-safe, so the IO hop bought nothing and theJobmade the read cancellable process-wide by accident), and the four long-deprecated*Boolmethods are gone.Compatibility
Most subclasses need no change. The existing 1,621-line test suite passes completely unchanged — the convenience constructor is source-compatible. Only call sites that explicitly passed
coroutineContextneed editing.Full migration guide in the README under
## Migrating to 2.0.The one that needs care
Making
suspendfunctions properly caller-cancellable is a genuine behaviour change, not a refactor. A logout-style sequence running on a screen-scoped coroutine can now be interrupted halfway, leaving SharedPreferences cleared and DataStore not. Anything that relied on a prefs write surviving its caller's cancellation needs an application-lifetime scope orwithContext(NonCancellable).Verification
:app:testDebugUnitTest— green, including the untouched existing suite:app:koverVerifyDebug— coverage floor holds:PrefsHelper:build— greenNew
BaseDataStoreHelperInjectionTest(7 tests, 3.7s, no sleeps or polling) covers the injected DataStore, the injected scope, per-instance scope isolation, and caller-cancellableclearPrefson both helpers.The two cancellation tests were checked against a mutated build: reverting
clearPrefsto a foreign-JobwithContextmakes exactly those two fail and nothing else, so they're real regression guards rather than tests that pass either way.Notes for the reviewer
master—dokkaHtmlfails with "Cannot run Dokka V1 tasks when V2 mode is enabled", since Dokka 2.2.0 (already pinned before this branch) removed the V1 task. Corrected the command inCLAUDE.mdtodokkaGenerateHtml, which builds cleanly. Worth checking nothing in CI still calls the old one.*Boolmethods wereprotected, so only subclasses can break — grep the consuming apps for them alongside thecoroutineContext/supervisorJobsweep.Not in this PR
🤖 Generated with Claude Code