docs: bannerBuilder cannot return null — document how to hide the banner - #27
Conversation
… banner (#25) The docstring told callers to 'return null to hide the banner', but the nullable ? is on the FUNCTION, not its return: (c, r, d) => null does not compile, and bannerBuilder: null selects the default banner. The call site feeds the result straight into Positioned(child: ...), so a null return could never be tolerated either. Ship the doc the type can keep (issue option 1): the builder must return a widget; SizedBox.shrink() is how you render nothing while driving your own update UI from CodePush.checkForUpdate or CodePush.status; onDismiss hides the banner slot until the next ready update. The docs site already describes this behavior (code-push-website #25), so no follow-up is created there.
🔴 CriticalNone. This is doc + CHANGELOG only — no behavior or API-signature change, so nothing here can break existing users or the update path. The core claim of the PR checks out: 🟠 Medium
🟡 Low
🟢 Positives
|
…DME copy; tighten onDismiss wording The round-1 doc steered callers to CodePush.checkForUpdate, which talks to the engine channel and rethrows as CodePushException on engines without code push — a sharp edge exactly where the doc is most needed. Listen to CodePush.status instead, as the overlay itself does. README's bannerBuilder example (the third copy of this doc) gains the same must-return-a-widget line; 'until the next ready update' is tightened to 'until the next NEW patch becomes ready' (an installed patch pending restart short-circuits re-offers of itself).
|
Round-2 push
|
🔴 CriticalNone. Round 2 (
🟠 Medium
🟡 Low
Not re-raising: the 🟢 Positives
|
…s debug channel Round 2 steered readers at CodePush.status — which is failure-soft but is (a) a declared debug channel of free-form strings and (b) not what gates the banner: _updateReady flips only via the onUpdateReady callbacks on init/checkAndInstall. All three doc copies (docstring, README, CHANGELOG) now name onUpdateReady:, the docstring is split into three short paragraphs, and checkForUpdate's own doc carries the throws warning where readers of THAT symbol will actually see it.
|
Round-3 push answers round 2:
|
🔴 CriticalNone. Round 3 remains doc + CHANGELOG only — no signature change, no behavior change, the update/download/rollback path is untouched. The three round-2 items marked fixed are verified fixed:
🟠 Medium
🟡 Low
Not re-raising: the 🟢 Positives
|
…edes an earlier init Round 3's onUpdateReady: recommendation doesn't survive the overlay's own lifecycle: CodePushOverlay.initState calls CodePush.init itself, cancelling the timer and superseding a caller's callback (which then fires at most once, nondeterministically, via the single-flight race). The accurate contract: bannerBuilder is only invoked once a patch is installed and awaiting restart — the builder call IS the signal; apps that want to own the lifecycle drive init/checkAndInstall directly instead of the overlay. All three copies aligned; checkForUpdate's alternative note now says those calls install, unlike the check-only query.
|
Round-4 push: adopted your builder-is-the-signal framing across all three copies (docstring, README, CHANGELOG), added the overlay-supersedes-earlier- |
🔴 CriticalNone. Round 4 is still doc + CHANGELOG only — no signature change, no behavior change, the check/download/install/rollback path is untouched. The three round-3 items marked fixed are verified fixed:
🟠 Medium
🟡 Low
Not re-raising: the 🟢 Positives
|
…ersedes note; CHANGELOG split
|
Round 5 fixes pushed (
✅ |
🔴 CriticalNone. Round 5 is still doc + CHANGELOG only — no signature change, no behavior change, the check/download/install/rollback path is untouched. The three round-4 items marked fixed are verified fixed:
🟠 Medium
🟡 Low
Not re-raising: the 🟢 Positives
|
…bove MaterialApp needs navigatorKey
|
Round 6 fixes pushed (
CHANGELOG left as-is — its round-5 wording already says "keep side effects out of the builder" without claiming a throw, so it stays accurate. ✅ |
🔴 CriticalNone. Round 6 (
🟠 Medium
🟡 Low
Not re-raising: the 🟢 Positives
Merge gate: one open Medium. Per the policy it can merge if filed as its own issue labelled |
…estart is wire-up not invoke; placement-scoped Navigator caveat
|
Round 7 pushed (
✅ |
🔴 CriticalNone. Round 7 (
🟠 Medium
🟡 Low
Not re-raising: the 🟢 Positives
Merge gate: two open Mediums. Per the policy this can merge with each filed as its own issue labelled |
… doc + config; checkForUpdate points to hasCodePushEngine; honest example (file website #38)
|
Round 8 pushed ( Mediums
Lows
✅ |
🔴 CriticalNone. Round 8 (
🟠 Medium
🟡 Low
Not re-raising: the 🟢 Positives
Merge gate: two open Mediums. Per the policy this can merge with each filed as its own issue labelled |
…d is always-false on iOS); init-in-main races even without onUpdateReady
|
Round 9 pushed (
Lows:
✅ |
🔴 CriticalNone. Round 9 (
🟠 Medium
🟡 Low
Not re-raising: the 🟢 Positives
Merge gate: two open Mediums, so this can merge under the policy with each filed as its own issue labelled |
…duleResult has a null-notify hole); un-gate example Rollback (works on iOS); README quickstart iOS caveat; CHANGELOG checkForUpdate note
|
Round 10 pushed (
✅ |
🔴 CriticalNone. Round 10 ( 🟠 Medium1. The doc says
From there the comparison is Failure scenario: a developer follows the doc verbatim with a The fix is in this file already: the overlay does not key on the value, it latches on the transition — Same shape as #25 — a docstring instructing something the code cannot do — reintroduced by the fix for it, so worth closing rather than deferring. 2. Round 10 says it was "Applied to docstring + README + CHANGELOG", but 🟡 Low
🟢 Positives
Merge gate: two open Mediums — mergeable under the policy with each filed as its own Not re-raising: the Unreleased heading and the missing No Dart SDK in this environment, so |
… (not a level compare); fix status/isPatched README reference sections; drop private-field ref
|
Round 11 pushed (
Lows: dropped the private Tabling one Low — introducing a ✅ |
🔴 CriticalNone. Round 11 ( 🟠 Medium
The instruction is "LATCH a
Failure scenario: an iOS app follows the docstring verbatim — Note the asymmetry the mechanism creates, because it decides the fix: a level survives re-inflation, an edge does not. Whichever wording wins has to reach all six copies (see the second Low) — and the two that weren't updated this round happen to be the two that currently describe something that works. 🟡 Low
🟢 Positives
Merge gate: one open Medium. Per the policy it can merge if filed as its own issue labelled Not re-raising: the No Dart SDK in this environment, so |
…verlay re-key; the status edge/latch does not) — reverted latch guidance across all 6 copies; debug-bar sample = Restart to apply
|
Round 12 pushed ( Reverted to Lows: README debug-bar sample is now ✅ |
🔴 CriticalNone. Round 12 ( 🟠 Medium
🟡 Low
🟢 Positives
Merge gate: one open Medium — mergeable under the policy if filed as its own issue labelled Not re-raising: the No Dart SDK in this environment, so |
…with ValueListenableBuilder<Object?> (CodePushPatchBuilder needs a keyed String); add isPatched dartdoc iOS caveat + README moduleResult section + example rollback case
|
Round 13 pushed (
✅ |
🔴 CriticalNone. Round 13 ( 🟠 Medium1. The paragraph's subject is "an app-facing iOS signal". Both ways a reader can take that clause fail on iOS:
Failure scenario: an iOS app drops 2. Round 13 reports the pointer removed "from all three copies", but Platform Behavior still tells iOS readers the opposite: "Listen to Failure scenario: an iOS developer reads Platform Behavior, wraps patched UI in 🟡 Low
Not re-raising: the 🟢 Positives
Merge gate: two open Mediums, so this is mergeable under the policy with each filed as its own No Dart SDK in this environment, so |
… builder test - Remove the checkAndInstall fallback clause from status dartdoc (it never fires onUpdateReady on the iOS first-patch path — no app-facing signal). - Correct CodePushPatchBuilder contract in three copies (widget dartdoc + two README passages): only *string* results pass through; a Map/List payload yields the baseline branch. Delimiter is a colon, not a pipe. - moduleResult caveat now names both null cases (pure code patch; revert to baseline via rollback or an automatic post-failure revert). - Widen the example Rollback handler to catch FileSystemException from the iOS Dart-side patch delete, not just CodePushException. - Add a widget test locking the CodePushPatchBuilder pass-through contract.
🔴 CriticalNone. Round 14 (
🟠 Medium1. The comment says
Failure scenario: a developer pastes the example, points it at a server that 500s (or ships a patch whose hash fails verification), taps Check for Updates, and is told there is no update. The reason is sitting in This is the same "an enumeration that isn't exhaustive reads as a contract" shape the PR exists to fix, in newly-added text. One-line fix that makes the example strictly better than any enumeration: if (!installed) {
// `false` is not "no update" — it is also "a check is already running",
// "download failed", "hash mismatch", "server error". The reason is in
// CodePush.status, which checkAndInstall writes before every false return.
setState(() => _status = 'No new patch installed: ${CodePush.status.value}');
}2.
Failure scenario: identical to round-13's Medium 2, which the author accepted — a developer writes 🟡 Low
Not re-raising: the 🟢 Positives
Merge gate: two open Mediums — mergeable under the ≤2 policy provided each is filed as its own issue labelled No Dart SDK in this environment, so |
Comprehensive pass over all copies the review flagged as out of sync: - CodePushPatchBuilder: correct the builder-field dartdoc and class-level dartdoc to the string-only contract (were still general-purpose); fix 'every string' -> 'every non-empty string' in the field dartdoc and README (an empty string yields the baseline branch, as the new test pins). - moduleResult null caveat now names both cases (no-return-value patch AND revert-to-baseline) in every remaining copy: bannerBuilder dartdoc and both README passages. - Example: the 'false' branch surfaces CodePush.status instead of guessing 'no update' (checkAndInstall returns false from many failure sites), and a ValueListenableBuilder<Object?> over moduleResult is now shown, since the example's own note prescribes it as the iOS signal. - CHANGELOG: sanitize the Unreleased section to minimal, user-facing notes (dropped internal overlay-re-key/latch/edge language per the CHANGELOG minimalism rule), which also removes the drifting null-case caveat.
🔴 CriticalNone. Round 15 is doc/comment-only apart from Round-14 items verified fixed at the source: the 🟠 Medium1. Of the eighteen Failure scenario: the overlay's launch check (or the resume check at
2.
Failure scenario (ordinary iOS cold start with a resident patch): 🟡 Low
Not re-raising: the 🟢 Positives
Merge gate: two open Mediums — mergeable under the ≤2 policy provided each is filed as its own No Dart SDK in this environment: |
|
Table-and-merge (round 16), per the ≤2-Medium gate. Round 16 is 0 Critical / 2 Medium / 5 Low. Both Mediums are about the example code this round added, and — as the review itself notes — every Medium for several rounds has been a missed copy, never a new wrong claim; the SDK library docs (the deliverable) are now correct and consistent. Both Mediums are filed as
Lows tabled (each a one-clause/example-only nit); the contract-pinning test ( |
Fixes #25 via the issue's option 1 (docstring fix — the smaller change, and the one matching what callers can do today).
The old docstring promised
Return null to hide the banner, but the?is on the function type, not its return:(c, r, d) => nulldoes not compile,bannerBuilder: nullselects the default banner, and the call site feeds the result straight intoPositioned(child: …). There was no way to do what the doc instructed.New doc states: the builder must return a widget; return
const SizedBox.shrink()to show nothing while driving your own update UI (CodePush.checkForUpdate/CodePush.status);onDismisshides the banner slot until the next ready update (verified against the_updateReadylatch at the call site).Why not option 2 (
Widget? Function(...)?+ call-site guard): it is the nicer API, but it is a public API semantic change on a published package, and the docs site was already corrected to describe option-1 behavior in code-push-website #25 — shipping option 2 would re-open that row. If option 2 is wanted later it can supersede this cleanly; the issue stays the record of that choice.Doc + CHANGELOG only; no behavior change.
dart analyze: the 6 info-lints are pre-existing onmain(verified by stash).