Skip to content

Conversation

@MiaKoring
Copy link
Contributor

This PR adds:

  • EnvironmentValues/with(key:, value:) //equivalent to with(keyPath: newValue:) but for non-builtin values
  • view modifiers for both builtin (key path) and not builtin (EnvironmentKey) values

For demonstration and testing purposes I changed GreetingGeneratorExample’s latest greeting display to get its value over an environment key.

The @Environment property wrapper was changed to additionally support EnvironmentKeys.

I did some limited testing which worked like expected. The modifiers just use the EnvironmentModifier View just like we do with most other modifiers.

Copy link
Owner

@stackotter stackotter 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 these changes! They will make working with custom environment values much easier.

While reviewing the PR, I googled stuff about SwiftUI's environment modifiers, and I found that SwiftUI works around the distinction between custom keys and builtin environment properties (keypaths) by instructing users to extend EnvironmentValues with their own custom properties that utilise the environment key. Your implementation doesn't stop users from taking that approach, but I'm wondering whether we should try to push users towards the SwiftUI approach, or instead let them choose.

I was leaning towards letting users choose because it makes defining custom environment values very simple (just define the key). However, I realised that having these two different types of environment properties makes it tricky for libraries to provide methods that are generic over environment properties, because they would need to vend two separate (but very similar) methods for each abstraction.

For that reason, I think that it's probably best to not have the environment key based methods? But I'm open to discussion on that.

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