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

wip: Expose ComposableEnvironment.updatingFromParentIfNeeded to public API #4

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

maximkrouk
Copy link
Contributor

@maximkrouk
Copy link
Contributor Author

I refactored code a bit (probably it will also improve the performance, since there is no need to scope the environment for each action, only to check if the reducer is not nil) by creating a lazy reducer

extension Reducer where Environment: ComposableEnvironment {
  public static func recursive<
    LocalState,
    LocalAction,
    LocalEnvironment: ComposableEnvironment
  >(
    _ reducer: @escaping () -> Reducer<LocalState, LocalAction, LocalEnvironment>,
    state toLocalState: WritableKeyPath<State, LocalState>,
    action toLocalAction: CasePath<Action, LocalAction>
  ) -> Reducer {
    return Reducer<LocalState, LocalAction, LocalEnvironment>
      .lazy(reducer)
      .pullback(state: toLocalState, action: toLocalAction)
  }
}

extension Reducer {
  public static func lazy(_ reducer: @escaping () -> Reducer) -> Reducer {
    var _reducer: Reducer!
    return Reducer { state, action, environment in
      if _reducer.isNil { _reducer = reducer() }
      return _reducer.run(&state, action, environment)
    }
  }
}

Anyway, it might be useful, so it's up to you if the PR makes sense 🙂

@tgrapperon
Copy link
Owner

Hello @maximkrouk, thanks for the PR. As I understand it, you don't need the method to be public anymore thanks to the overload you're showing for .recursive with LocalEnvironment: ComposableEnvironment. Am I right?

Maybe some function that would allow to instantiate a derived environment of some type from an existing parent environment would be useful too. I'm a little concerned over the naming if updatingFromParentIfNeeded would be public. Maybe something more explicit is preferable. Ultimately, a weak parent property may be used (I'm not seeing unexpected side effect right now). To sum up, something like:

let child1 = parent1.extract(ChildEnvironment.self)

child2.parent = parent2

would both produce a couple of correctly linked parent/child.
What do you think about this?

@maximkrouk
Copy link
Contributor Author

Yep, I solved it without this method, anyway I still want to investigate a little bit more about recursion, maybe it can be optimized even further.

+1 for the naming, it probably should be prettified for the public API (for the internal use I find it explicit in a good way)

I'll also take a closer look at sources tomorrow to give you feedback about parent-child relationship.

What do you think about getting rid of updates, but keeping the same reference to dependencies storage? Afaiu strait-forward implementation will break with feature, so maybe there may also be local storage for overridden dependencies... Or dependencies storage may be a separate graph-object, to be up-to date just by it's structure, rather then imperative updates, I'll think about it tomorrow too 🙂

@maximkrouk
Copy link
Contributor Author

maximkrouk commented Dec 24, 2021

Tried to implement ComposableDependencies as a graph, but seems like it works a bit slower (tested manually on testModifyingDependenciesOncePrimed function with measure, in general it's about 250ms vs 300ms, maybe it's not representative)

public final class _ComposableDependencies {
  weak var context: _ComposableDependencies?
  var values: [ObjectIdentifier: Any] = [:]
  
  public subscript<T>(_ key: T.Type) -> T.Value where T: DependencyKey {
    get { values[ObjectIdentifier(key)] as? T.Value ?? context?[key] ?? key.defaultValue } // search for overrides,if not found ask parent, if no parent or overrides then return default value
    set { values[ObjectIdentifier(key)] = newValue }
  }
}

open class ComposableEnvironment {
  public required init() {}
  
  var _dependencies = _ComposableDependencies()
  
  @discardableResult
  func connected(to env: ComposableEnvironment) -> Self {
    self._dependencies.context = env._dependencies // doesn't modifies dependencies, just connects parent as a context to self
    return self
  }
  
  // ...
}

public final class _DerivedEnvironment<Value> where Value: ComposableEnvironment {
  public static subscript<EnclosingSelf: ComposableEnvironment>(
    _enclosingInstance instance: EnclosingSelf,
    wrapped wrappedKeyPath: ReferenceWritableKeyPath<EnclosingSelf, Value>,
    storage storageKeyPath: ReferenceWritableKeyPath<EnclosingSelf, _DerivedEnvironment>
  ) -> Value {
    get {
      instance[keyPath: storageKeyPath]
        .environment
        .connected(to: instance) // stays the same, except of updatingFromParent replaced with connected(to:)
    }
    set {
      fatalError("@DerivedEnvironments are read-only in their parent")
    }
  }
  
  // ...
}

public struct _Dependency<Value> {
  public static subscript<EnclosingSelf: ComposableEnvironment>(
    _enclosingInstance instance: EnclosingSelf,
    wrapped wrappedKeyPath: ReferenceWritableKeyPath<EnclosingSelf, Value>,
    storage storageKeyPath: ReferenceWritableKeyPath<EnclosingSelf, Self>
  ) -> Value {
    get {
      let wrapper = instance[keyPath: storageKeyPath]
      let keyPath = wrapper.keyPath
      let value = instance._dependencies[keyPath: keyPath] // stays the same, just an underscore added here
      return value
    }
    set {
      fatalError("@Dependency are read-only in their ComposableEnvironment")
    }
  }
  
  // ...
}

So this way I guess

  • Dependencies become a bit more strait-forward to understand
  • Environment should not know about its parent, the dependency graph is self-searching
  • Afaiu envionments were already optimized to not be recreated by capturing them in reducers, so even recursion handling with a lazy reducer is fine and should not create problems already even without the PR

@maximkrouk maximkrouk changed the title Expose ComposableEnvironment.updatingFromParentIfNeeded to public API wip: Expose ComposableEnvironment.updatingFromParentIfNeeded to public API Dec 24, 2021
@maximkrouk
Copy link
Contributor Author

Pushed changes, marked PR as wip

@maximkrouk maximkrouk marked this pull request as draft December 24, 2021 14:55
@maximkrouk
Copy link
Contributor Author

I don't really remember the point of this PR, seems like it's too outdated anyway and I don't need this feature since the project died 💀 so I'm just closing it 🙂

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