Open piotrekjeremicz opened 3 weeks ago
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 Group
contained other groups, of which only some had their own font()
modifier set, the environment should trickle down correct in a hierarchical way.
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 CoreAttributes
struct, so that each element type can have various environment overrides set. I have also modified the built-in element types so they forward their environment values onto their children correctly, taking into account overrides – "give my font to my children, but only if they don't specify a custom font of their own."
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 possible-environment-solution
branch. I'm afraid it was created a while ago, so it's actually behind the main
branch in quite a few places, but that's a problem we can solve once we figure out the best way forward.
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.
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.