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.

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

Enable customDump only in debug mode #3058

Merged
merged 2 commits into from
May 9, 2024

Conversation

heoblitz
Copy link
Contributor

@heoblitz heoblitz commented May 8, 2024

Reduce unnecessary code execution by enabling customDump only in debug mode, as in .run Effect.

https://github.com/pointfreeco/swift-composable-architecture/blob/main/Sources/ComposableArchitecture/Effect.swift#L101

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to PR! The runtimeWarn helper is already lazy, but will still emit failures in release tests. In order to retain that behavior, let's not #if DEBUG stuff entirely. Instead, can you move that work into the runtimeWarn message argument?

Comment on lines 194 to 225
#if DEBUG
var actionDump = ""
customDump(action, to: &actionDump, indent: 4)
var stateDump = ""
customDump(state, to: &stateDump, indent: 4)
runtimeWarn(
"""
An "ifCaseLet" at "\(self.fileID):\(self.line)" received a child action when child state \
was set to a different case. …

Action:
\(actionDump)
State:
\(stateDump)

This is generally considered an application logic error, and can happen for a few reasons:

• A parent reducer set "\(typeName(Parent.State.self))" to a different case before this \
reducer ran. This reducer must run before any other reducer sets child state to a \
different case. This ensures that child reducers can handle their actions while their \
state is still available.

• An in-flight effect emitted this action when child state was unavailable. While it may \
be perfectly reasonable to ignore this action, consider canceling the associated effect \
before child state changes to another case, especially if it is a long-living effect.

• This action was sent to the store while state was another case. Make sure that actions \
for this reducer can only be sent from a view store when state is set to the appropriate \
case. In SwiftUI applications, use "SwitchStore".
"""
)
#endif
Copy link
Member

Choose a reason for hiding this comment

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

We can use some helpers to inline the work:

Suggested change
#if DEBUG
var actionDump = ""
customDump(action, to: &actionDump, indent: 4)
var stateDump = ""
customDump(state, to: &stateDump, indent: 4)
runtimeWarn(
"""
An "ifCaseLet" at "\(self.fileID):\(self.line)" received a child action when child state \
was set to a different case.
Action:
\(actionDump)
State:
\(stateDump)
This is generally considered an application logic error, and can happen for a few reasons:
A parent reducer set "\(typeName(Parent.State.self))" to a different case before this \
reducer ran. This reducer must run before any other reducer sets child state to a \
different case. This ensures that child reducers can handle their actions while their \
state is still available.
An in-flight effect emitted this action when child state was unavailable. While it may \
be perfectly reasonable to ignore this action, consider canceling the associated effect \
before child state changes to another case, especially if it is a long-living effect.
This action was sent to the store while state was another case. Make sure that actions \
for this reducer can only be sent from a view store when state is set to the appropriate \
case. In SwiftUI applications, use "SwitchStore".
"""
)
#endif
runtimeWarn(
"""
An "ifCaseLet" at "\(self.fileID):\(self.line)" received a child action when child state \
was set to a different case.
Action:
\(String(customDumping: action).indent(by: 4))
State:
\(String(customDumping: state).indent(by: 4))
This is generally considered an application logic error, and can happen for a few reasons:
A parent reducer set "\(typeName(Parent.State.self))" to a different case before this \
reducer ran. This reducer must run before any other reducer sets child state to a \
different case. This ensures that child reducers can handle their actions while their \
state is still available.
An in-flight effect emitted this action when child state was unavailable. While it may \
be perfectly reasonable to ignore this action, consider canceling the associated effect \
before child state changes to another case, especially if it is a long-living effect.
This action was sent to the store while state was another case. Make sure that actions \
for this reducer can only be sent from a view store when state is set to the appropriate \
case. In SwiftUI applications, use "SwitchStore".
"""
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for checking! I've removed the EXP flag and modified it to pass as an autoclosure parameter directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BindableActionViewStoreDebugger implementation is within the EXP flag, and since maxDepth configuration is not possible, I've excluded it for now.

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

Thanks!

@stephencelis stephencelis merged commit 1792402 into pointfreeco:main May 9, 2024
7 checks passed
cgrindel-self-hosted-renovate bot added a commit to cgrindel/rules_swift_package_manager that referenced this pull request May 13, 2024
…ure to from: "1.10.4" (#1067)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[pointfreeco/swift-composable-architecture](https://github.com/pointfreeco/swift-composable-architecture)
| patch | `from: "1.10.3"` -> `from: "1.10.4"` |

---

### Release Notes

<details>
<summary>pointfreeco/swift-composable-architecture
(pointfreeco/swift-composable-architecture)</summary>

###
[`v1.10.4`](https://github.com/pointfreeco/swift-composable-architecture/releases/tag/1.10.4)

[Compare
Source](https://github.com/pointfreeco/swift-composable-architecture/compare/1.10.3...1.10.4)

#### What's Changed

- Fixed: Compute warnings lazily by
[@&#8203;heoblitz](https://github.com/heoblitz) in
[pointfreeco/swift-composable-architecture#3058
- Fixed: Lazy loading of Shared initial values by
[@&#8203;seanmrich](https://github.com/seanmrich) in
[pointfreeco/swift-composable-architecture#3060
- Fixed: Fix for throttle logic. by
[@&#8203;mbrandonw](https://github.com/mbrandonw) in
[pointfreeco/swift-composable-architecture#3075

#### New Contributors

- [@&#8203;heoblitz](https://github.com/heoblitz) made their first
contribution in
[pointfreeco/swift-composable-architecture#3058

**Full Changelog**:
pointfreeco/swift-composable-architecture@1.10.3...1.10.4

</details>

---

### Configuration

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

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- 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:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDkuNCIsInVwZGF0ZWRJblZlciI6IjM2LjEwOS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants