Skip to content

Conversation

josefdolezal
Copy link
Collaborator

Pull Request Type

  • Bug fix
  • New feature
  • Refactoring
  • Documentation update
  • Chore

Issue for this PR

Link: N/A

Description

Fixes issue where effects with dependencies (eg .preserved(by:)) would cleanup itself after new effect ran. This is different behavior from React's implementation and could cause unexpected results.

Motivation and Context

Running the following code with Hooks v0.6.0, after tapping the button, useHook would first run new effect, then cleanup after previous effect. The timeline is:

  1. [Key: 1]
  2. Run Effect [1]
  3. Increment key [Key: 2]
  4. Run Effect [2]
  5. Cleanup Effect [1]

Steps 4 and 5 should run in opposite order.

The issue emerges in UseEffectHook.State in willSet property observer. The observation triggers old effect's cleanup by oldValue?() by which time new effect already finished to return new cleanup closure.

SwiftUI Code
func App() -> some View {
    HookScope {
        let state = useState(0)
        let _ = useEffect(.preserved(by: state.wrappedValue)) {
            print("subscribe \(state.wrappedValue)")
            return {
                print("unsubscribe \(state.wrappedValue)")
            }
        }

        Text(state.wrappedValue.description)
        Button("Add") {
            state.wrappedValue += 1
        }
    }
}
React Code
function App() {
  const [state, setState] = React.useState(0);

  useEffect(() => {
    console.log("subscribe " + state);

    return () => console.log("unsubscribe " + state);
  }, [state]);

  return (
    <div>
      <p>{state}</p>
      <button onClick={() => setState((old) => old + 1)}>Add</button>
    </div>
  );
}

Running the sample code in patch-1 branch, the results are same as React app above.

Impact on Existing Code

The change is source-compatible.

Screenshot/Video/Gif

@josefdolezal josefdolezal changed the title Cleanup after effect before executing new one Cleanup after an effect before executing a new one Jun 16, 2022
Comment on lines -76 to -78
var cleanup: (() -> Void)? {
didSet { oldValue?() }
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: Updating code to willSet is not enough, because effect() was already executed.

@testable import Hooks

final class UseEffectTests: XCTestCase {
enum EffectOperation: Equatable {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Open for naming suggestions 🙏

Copy link
Owner

@ra1028 ra1028 left a comment

Choose a reason for hiding this comment

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

@josefdolezal
Good point, it looks pretty good to me! Thanks 🚀

@ra1028 ra1028 merged commit 12885ab into ra1028:main Jun 24, 2022
@ra1028
Copy link
Owner

ra1028 commented Jun 24, 2022

https://github.com/ra1028/swiftui-hooks/releases/tag/0.0.7

@josefdolezal josefdolezal deleted the patch-1 branch June 24, 2022 13:36
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.

2 participants