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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declare DependencyKeys namespace #5

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

Conversation

maximkrouk
Copy link
Contributor

@maximkrouk maximkrouk commented Dec 24, 2021

I find it pretty convenient to declare dependency keys in a namespace. I can implement it in my higher-level package (tca-extensions), but I guess that tca-env is a more appropriate place for this thing 馃檪.

Note that Readme is updated a bit

@tgrapperon
Copy link
Owner

Thanks for the PR @maximkrouk!

I'm a little wary about the additional complexity DependencyKeys may introduce. A DependencyKey can be completely private and defined on the spot before implementing the computed property in ComposableDependencies. You can even use the type of the dependency itself if this is something unique. For example, if you have a DataStore dependency, you can make it conform to DependencyKey (you only need to provide a default value), and use the type itself as a dependency key when defining your computed property. Introducing such a namespace may introduce some rigidity about the way users are declaring their properties (or made them think they should always use the DependencyKeys namespace), and I'm not seeing any clear benefit in having such a global and unique transitive namespace. Do you have some example where it makes sense?

The extension to the README is great! I would also like to preserve a simple example (that's why I've used mainQueue which is quite common among TCA users). Obviously, the mainQueue collides with the EventHandlingScheduler function, and it can be confusing, so I think we can present the function in two steps: we start with .mainQueue which bears no mental overload to introduce DependencyKey and ComposableDependencies, and later in the README, we present a more real-life configuration, with connected environments and more complex dependencies with .live variants, etc. Would it be OK with you?

Furthermore, the current version of README is a little outdated, especially regarding the latest AutoComposableEnvironment , and I was thinking about reorganizing it. I'll think about it this weekend.

@maximkrouk
Copy link
Contributor Author

Well, keeping keys private feels reasonable. Keeping README simple feels reasonable, maybe it is worth reverting it and updating the example a bit. Currently, I'm thinking about improving my prev. PR, so I'll update this one a bit later c:

@maximkrouk maximkrouk marked this pull request as draft December 24, 2021 14:56
- reverted README.md
- implemented more complex schedulers management in example
- did some refactoring
  - indentation
  - type inference
  - moved comments to a higher code level to keep declarations smaller
@tgrapperon
Copy link
Owner

Thanks @maximkrouk!
I'm working on some consequent refactor of the library. Basically, the user will have two choices: ComposableEnvironment with some SwiftUI-like inheritance, and GlobalEnvironment with shared global access to dependencies. This last one will be able to be used with structs environments, with access to global dependencies. Switching from Composable to Global should be source-compatible, and the recommended way to go if users are not updating their dependencies in the middle of the chain.
As a result, it may change the way things will be presented and their tradeoffs, so it may be worth retraining a little your efforts on this PR and #4 until this is released.

@maximkrouk
Copy link
Contributor Author

Fine 馃檪. Also, +1 for the global environment should be pretty convenient feature for UIKit

I guess it might be easily implemented using my second PR, each root environment could use shared ComposableDependencies object (which could be accessible using trough GlobalEnvironment object))

Afaiu now it works in a similar way, if you create some environment (without anything in it's body) you can access all of the ComposableDependencies. Am I right? Or there is some different concept?

@tgrapperon
Copy link
Owner

tgrapperon commented Dec 24, 2021

@maximkrouk I've pushed the value-type branch as a first draft, so you can get more precise answers. But yes, if all the dependencies are global, the environment can be empty. You conform to GlobalDependencyAccessing to get implicit KeyPath subscript (or you can use the @Dependency property wrapper as usual). But if it's empty, you can furthermore conforms to GlobalEnvironment and get access to environment-less pullbacks. Unfortunately, you may need to explicitely provide a public init() {} if your type is a struct. I'm trying to polish this point, but I don't think it's possible.

It defines a GlobalEnvironment module that can be used on new projects, and a GlobalEnvironmentCompat module to provide source compatibility for users coming from ComposableEnvironment. It's mostly a matter of aliasing Composable names to Global names. For example, ComposableDependencies is called Dependencies in GlobalEnvironment, as it wouldn't make sense otherwise. I haven't tried yet, but switching from ComposableEnvironment to GlobalEnvironmentCompat should only require to fix the imports. Right now, ComposableEnvironment and GlobalEnvironment are mutually exclusive modules.

The @Dependency property wrapper from GlobalEnvironment can be installed anywhere btw, so it may open more convenient uses from external dependencies.

I'll elaborate on the documentation in the upcoming days.
[Edit: After a second thought, ComposableDependencies will be deprecated in favor of a unique Dependencies]

@maximkrouk
Copy link
Contributor Author

I'm not sure that it's really crucial, but I liked the option to override dependencies in the ComposableEnvrionment, afaiu it's not possible in GlobalEnvrionment due to direct access to Dependencies.global, so any override seems to be global too.

@maximkrouk
Copy link
Contributor Author

Oh, I see you pushed DependencyAliases, that fixes my concern from the prev reply, I guess

@tgrapperon
Copy link
Owner

I liked the option to override dependencies in the ComposableEnvrionment

Then you can still use ComposableEnvironment! In order to store the eventual overrides mid-chain in a convenient way, all your environment must be some subclass of ComposableEnvironment. This is a very strong requirement, for a reason that is not exploited in many cases. If you relax the mid-chain overrides specification, you get GlobalEnvironment, with requirements so weak that you can even opt-in anywhere, without the whole chain being typed as something, or conforming to some protocol. This should lower the level of entry for many users. If at some point, they need the customization, they can switch to ComposableEnvironment, without having to touch the code declaring and accessing dependencies.

@tgrapperon
Copy link
Owner

I see you pushed DependencyAliases, that fixes my concern from the prev reply, I guess

I think so too. It is still recommended to use some top-level definitions, but if it's not possible, you can stitch the pieces and let the library synchronize the values. This will need real-life testing though.

@tgrapperon
Copy link
Owner

I'll soon push GlobalDependencies and aliases on main. I've rewritten the README almost totally.
There is no example in it because I knew you had a nice one ready to go.
Feel free to PR modifications on the README once it's live.
It may be better to open a separate PR for this though, as many things have changed in the meantime.

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