Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Short circuit rememberRetained if the LocalRetainedStateRegistry isn't set #1438

Merged
merged 2 commits into from
May 28, 2024
Merged

Short circuit rememberRetained if the LocalRetainedStateRegistry isn't set #1438

merged 2 commits into from
May 28, 2024

Conversation

stagg
Copy link
Collaborator

@stagg stagg commented May 28, 2024 •
edited
Loading

Fixes #1434

ZacSweers
ZacSweers approved these changes May 28, 2024
View reviewed changes
Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

This makes sense to me but CCing @alexvanyo just in case for a second opinion

alexvanyo
alexvanyo reviewed May 28, 2024
View reviewed changes
circuit-retained/src/commonMain/kotlin/com/slack/circuit/retained/RememberRetained.kt
@@ -144,7 +145,10 @@ public fun <T : Any> rememberRetained(
): T {
val saveableStateRegistry = LocalSaveableStateRegistry.current
val retainedStateRegistry = LocalRetainedStateRegistry.current

// Short-circuit no-ops
if (retainedStateRegistry === NoOpRetainedStateRegistry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The likelihood of this is probably small, but would this do weird things if the value of LocalRetainedStateRegistry.current changed between being a NoOpRetainedStateRegistry and a functional one, or vice versa? I think that could result in lost state because the code paths would be entirely different?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure it'd be possible, NoOpRetainedStateRegistry is internal and is used as the default. So we'd be the ones jumping back and forth 😅 It would absolutely result in lost state if that did happen though.

@stagg stagg added this pull request to the merge queue May 28, 2024
Merged via the queue into main with commit 39cad58 May 28, 2024
5 checks passed
@stagg stagg deleted the js-fix-noop-retain branch May 28, 2024 21:47
stagg added a commit that referenced this pull request May 28, 2024
@stagg
Short circuit rememberRetained if the LocalRetainedStateRegistry …
421ed22 
…isn't set (#1438)

Fixes #1434
slack-oss-bot referenced this pull request in slackhq/foundry May 31, 2024
@slack-oss-bot
Update dependency com.slack.circuit:circuit-foundation to v0.22.1 (#865)
a84c97c 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[com.slack.circuit:circuit-foundation](https://github.com/slackhq/circuit)
| dependencies | minor | `0.20.0` -> `0.22.1` |

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>slackhq/circuit
(com.slack.circuit:circuit-foundation)</summary>

###
[`v0.22.1`](https://github.com/slackhq/circuit/blob/HEAD/CHANGELOG.md#0221)

[Compare
Source](https://github.com/slackhq/circuit/compare/0.22.0...0.22.1)

*2024-05-28*

- **Fix**: Fix `rememberRetained` implicitly requiring `LocalContext`
where it used to no-op.
-   Update Molecule to `2.0.0`.

###
[`v0.22.0`](https://github.com/slackhq/circuit/blob/HEAD/CHANGELOG.md#0220)

[Compare
Source](https://github.com/slackhq/circuit/compare/0.21.1...0.22.0)

*2024-05-28*

-   Update to Kotlin `2.0.0`.
-   Update to KSP `2.0.0-1.0.21`.
-   Update Compose Multiplatform to `1.6.10`.
-   Switch to the compose compiler shipped with Kotlin.

This release is otherwise identical to `0.21.0`, just updated to Kotlin
2.0.

###
[`v0.21.1`](https://github.com/slackhq/circuit/releases/tag/0.21.1)

[Compare
Source](https://github.com/slackhq/circuit/compare/0.21.0...0.21.1)

- **Fix**: Fix `rememberRetained` implicitly requiring `LocalContext`
where it used to no-op
([#&#8203;1438](https://github.com/slackhq/circuit/issues/1438))

#### What's Changed

- Short circuit `rememberRetained` if the `LocalRetainedStateRegistry`
isn't set by [@&#8203;stagg](https://github.com/stagg) in
[https://github.com/slackhq/circuit/pull/1438](https://github.com/slackhq/circuit/pull/1438)

**Full Changelog**:
slackhq/circuit@0.21.0...0.21.1

###
[`v0.21.0`](https://github.com/slackhq/circuit/blob/HEAD/CHANGELOG.md#0210)

[Compare
Source](https://github.com/slackhq/circuit/compare/0.20.0...0.21.0)

*2024-05-28*

-   **New**: Add WASM targets.
- **New**: Add `FakeNavigator` functions to check for the lack of
pop/resetRoot events.
- **New**: Add `FakeNavigator` constructor param to add additional
screens to the backstack.
- **New**: Add support for static UIs. In some cases, a UI may not need
a presenter to compute or manage its state. Examples of this include UIs
that are stateless or can derive their state from a single static input
or an input \[Screen]'s properties. In these cases, make your *screen*
implement the `StaticScreen` interface. When a `StaticScreen` is used,
Circuit will internally allow the UI to run on its own and won't connect
it to a presenter if no presenter is provided.
- **New**: Add `RecordLifecycle` and `LocalRecordLifecycle` composition
local, allowing UIs and presenters to observe when they are 'active'.
Currently, a record is considered 'active' when it is the top record on
the back stack.
- **New**: Add a `rememberRetainedSaveable` variant that participates in
both `RetainedStateRegistry` and `SaveableStateRegistry` restoration,
allowing layered state persistence.
- The logic is the following upon `rememberRetainedSaveable` entering
composition:
- consume from both `RetainedStateRegistry` and `SaveableStateRegistry`,
if available
        -   if the retained value is available, use that
- otherwise, if the saveable restored value is available, use that
        -   otherwise, re-initialize the value
- There is also an overload of `rememberRetained` that explicitly
requires a `Saver` parameter.
- **Behaviour Change**: Presenters are now 'paused' when inactive and
replay their last emitted `CircuitUiState` when they are not active.
Presenters can opt-out of this behavior by implementing
`NonPausablePresenter`.
- **Behaviour Change**: `NavigatorImpl.goTo` no longer navigates if the
`Screen` is equal to `Navigator.peek()`.
- **Behaviour Change**: `Presenter.present` is now annotated with
`@ComposableTarget("presenter")`. This helps prevent use of Compose UI
in the presentation logic as the compiler will emit a warning if you do.
Note this does not appear in the IDE, so it's recommended to use
`allWarningsAsErrors` to fail the build on this event.
- **Behaviour Change**: Guard against `Navigator.goTo()` calls to the
same current screen.
- **Change**: `Navigator.goTo` now returns a Bool indicating navigation
success.
- **Change**: Move iOS `GestureNavigationDecoration` impl to
`commonMain` and rename to `CupertinoGestureNavigationDecoration`.
-   **Change**: Target jvmTarget `1.8` in core libraries.
- **Fix**: Fix saveable state being restored when using reset root
navigation events.
- **Deprecation**: `FakeNavigator.assertIsEmpty` and `expectNoEvents`
(use the specific event type methods instead)
-   Mark `Presenter.Factory` as `@Stable`.
-   Mark `Ui.Factory` as `@Stable`.
-   Mark `CircuitContext` as `@Stable`.
-   Mark `EventListener` as `@Stable`.
-   Mark `EventListener.Factory` as `@Stable`.
-   \[samples] Improve interop sample significantly.
-   Update Kotlin to `1.9.24`.
-   Update KSP to `1.9.24-2.0.20`.
-   Update compose-compiler to `1.5.14`.
-   Update KotlinPoet to `1.17.0`.
-   Update androidx.lifecycle to `2.8.0`.
-   Update Molecule to `1.4.3`.
-   Update androidx.annotation to `1.8.0`.
-   Update kotlinx.coroutines to `1.8.1`.
-   Update Compose Multiplatform to `1.6.2`.
-   Update Compose UI to `1.6.7`.
-   Update Compose Runtime to `1.6.7`.
-   Update Compose Animation to `1.6.7`.
-   Update Compose Material to `1.6.7`.
-   Update androidx.core to `1.13.1`.
-   Update androidx.activity to `1.9.0`.
-   Update dagger to `2.51.1`.
-   Update uuid to `0.8.4`.

Special thanks to [@&#8203;chrisbanes](https://github.com/chrisbanes),
[@&#8203;alexvanyo](https://github.com/alexvanyo),
[@&#8203;eboudrant](https://github.com/eboudrant),
[@&#8203;edenman](https://github.com/edenman), and
[@&#8203;JustinBis](https://github.com/JustinBis) for contributing to
this release!

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zODEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjM4MS43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexvanyo alexvanyo alexvanyo left review comments

@ZacSweers ZacSweers ZacSweers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rememberRetained now implicitly required LocalContext in tests rather than no-op
3 participants
@stagg @ZacSweers @alexvanyo