TokamakUI / Tokamak

SwiftUI-compatible framework for building browser apps with WebAssembly and native apps for other platforms
Apache License 2.0
2.62k stars 111 forks source link

Fix `rootEnvironment` not merged with `.defaultEnvironment` #461

Closed regexident closed 2 years ago

regexident commented 2 years ago

While spelunking through the implementation of EnvironmentValues and its uses in Tokamak I noticed that a couple of functions were accepting a rootEnvironment: EnvironmentValues? = nil argument but then discarding whatever value you pass to it in favor of .defaultEnvironment.

This seems wrong to me? 🤔

carson-katri commented 2 years ago

Good catch. I think you may still need to add in the values .defaultEnvironment sets up if they are not already setup by the rootEnvironment though, specifically _ToggleStyleKey, .colorScheme, ._defaultAppStorage, and _DefaultSceneStorageProvider.

regexident commented 2 years ago

Let me know if you'd rather have the merging logic as a private implementation detail. I figured since effectively every backend/renderer implementation would need it it might be worth providing a canonical method for it. Providing both .merge(_:) and .merging(_:) might a bit overkill though.

carson-katri commented 2 years ago

Since this isn't part of SwiftUI, may be best to make it internal with @_spi so the renderers can access it but users don't.

regexident commented 2 years ago

Absolutely! Fixed.

regexident commented 2 years ago

Fixed!