-
Notifications
You must be signed in to change notification settings - Fork 64
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
Implement @Environment
property wrapper
#28
base: main
Are you sure you want to change the base?
Implement @Environment
property wrapper
#28
Conversation
I like this approach a lot |
I added tests. This evening I will calmly check the entire code again and release the PR! 🚀 |
I updated the description, ready to check! 😊 |
For folks reading this: @piotrekjeremicz and I have been exchanging emails about this since March 30th, so I'm not just ignoring them 🙂 Anyway, I'm moving it to this GitHub PR now, so it's all public. The goal of these changes is to implement the concept of "environment" in Ignite, which most folks are used to seeing in SwiftUI as a way of sharing information between views. The implementation in this PR is very good: it adds the Environment property wrapper, and the concept of environment keys and values. I'm grateful to @piotrekjeremicz for putting in such a lot of effort to make something really great – thank you! 🙌 I am concerned that the environment implementation here is globally shared rather than hierarchical, which means it would behave quite differently from SwiftUI. Specifically, I think folks would expect code like this work: Group {
Text("Whatever")
Text("Whatever")
Text("Whatever")
}
.font(.title1) From the perspective of a SwiftUI developer, I think it's fair to say they would expect that to set the font size for a given Group, and have that affect all things inside there at once – without changing the font elsewhere. Similarly, if that So, I'm worried that using a single, shared environment would preclude that entirely: it would use an Environment property wrapper like SwiftUI, but that would back on to a singleton behind the scenes, which I fear would cause confusion. I have attempted to create an alternative. It's rough, but at least it shows the direction of my thinking. This adds environment values to the I'm not saying my proposed solution is better, and in particular mine requires a more extensive change to the framework. However, it would also allow other modifiers to become environment modifiers – changing the text decoration, for example, would affect all child views at the same time, which again I think is closer to the way SwiftUI works. I have pushed up my version to the In earlier emails, @piotrekjeremicz linked to an excellent Swift Forums thread that dives into details about how SwiftUI might be implemented. When it comes to the environment, that in turn links to a talk I gave at iOS Conf SG three years ago, so I should probably rewatch that talk to see the approach I took back then 😅 Note: One thing to keep in mind in these discussions is that although I'd like our callsite/usage approach to be similar to SwiftUI where feasible, the implementation can be wildly different because we have very few run-time performance concerns. Ignite builds even huge sites in much less than a second, with no further performance impact once the HTML is generated. On the other hand, SwiftUI is executing code constantly as apps run, so their implementation is going to be heavily focused on performance optimization. |
@piotrekjeremicz Have you had a chance to read through the branch code? We're four weeks on now, so I'm keen to resolve this and move forward. You had said previously you'd be keen to work on some kind of |
@twostraws sorry for the delay in response. He is intensively preparing for WWDC. The approach with Your approach seems better because we get rid of singleton, and the whole thing is more coherent. |
@piotrekjeremicz Once we get this merged in, I'm hoping the next step would be to look at what else could use environment values too – I haven't investigated it, but I'd like to think the render context could be part of that, and if that's a requirement for you to get into the |
@twostraws I'd love to see what else I am more than happy if you merge your implementation. We will see where this story will take us! Still due to WWDC my time is severely limited. Less than a week left! See you! |
I'm going to ask the Swift team if this kind of thing can be asked in a lab. I don't want to waste their time with a question that's outside their remit! Same goes for the "some HTML" problem – if that's something they are happy to discuss, I'll try to boil them both down to simpler issues they can advise on. |
I think |
This Pull Request is adding
@Environment
property wrapper as a global environment injection.@Environment
was created based on the same equivalent from SwiftUI. It allows you to get a global variable, but the setter is not available outside the Ignite package. The first global variable implemented within this feature isPublishingContext
.You can use it as follows:
There are many global variables that could be added. It is simple, as in the original SwiftUI implementation. To add a new environment variable, first create a new
EnvironmentKey
and extendEnvironmentValues
with the declared getter and setter.EnvironmentValues
is stored in the memory once, as the singleton. This is a field for discussion if it is a good solution. Maybe a global variable could be better. In the case of the Ignite implementation, we have a static code generator. Memory optimization is primarily not our first priority.To set a value in a store, you can use the
shared
variable, as is presented below.My goal is to replace the creational function
func body(context: PublishingContext) -> [any BlockElement] {}
with a opaque typevar body: some HTML {}
as it stands in SwiftUI Views.