Add @EnvironmentObject #170

carson-katri · 2020-07-07T23:32:39Z

Currently changing the environment object doesn't redraw. What do you think the best way of triggering that would be @MaxDesiatov?

ie-ahm-robox · 2020-07-07T23:34:36Z

	Fails
🚫	Sources/TokamakCore/Environment/EnvironmentObject.swift#L27 - Line should be 100 characters or less: currently 121 characters (`line_length`)

Generated by 🚫 Danger Swift against ba17ce2

ie-ahm-robox · 2020-07-07T23:34:35Z

Sources/TokamakCore/Environment/EnvironmentObject.swift

+  func subscribe(_ closure: @escaping () -> ())
+}
+
+@propertyWrapper public struct EnvironmentObject<ObjectType>: EnvironmentReader, EnvironmentWriter where ObjectType: ObservableObject {


🚫 Line should be 100 characters or less: currently 135 characters (line_length)

MaxDesiatov · 2020-07-07T23:35:30Z

Hm, I was thinking of building @EnvironmentObject on top of @ObservedObject and adding support for the latter first?

MaxDesiatov · 2020-07-07T23:36:51Z

Also, @ObservedObject is incomplete in OpenCombine without OpenCombine/OpenCombine#97

carson-katri · 2020-07-07T23:38:09Z

In SwiftUI @EnvironmentObject uses the EnvironmentValues, which ObservedObject doesn't, so I think their implementations should be separate...

MaxDesiatov · 2020-07-07T23:40:19Z

Sorry, I mean on top of ObservableObject, which you already use, but working objectWillChange is based on some runtime tricks that only OpenCombine/OpenCombine#97 introduces.

MaxDesiatov · 2020-07-07T23:48:12Z

I see that you tried to merge that PR in your fork, you may find this issue somewhat useful with regards to compiling C++ dependencies swiftwasm/swift#595 (comment)

carson-katri · 2020-07-08T13:15:42Z

So I would need to pass those flags to the linker for it to compile the cpp? Or it’s just not in a working state?

MaxDesiatov · 2020-07-08T13:33:15Z

Most probably still not in a working state, unless it was fixed in LLVM in the meantime, need to have another look to clarify

carson-katri · 2020-07-08T13:34:58Z

Ok, because I tried to build and it failed while building std.

MaxDesiatov · 2020-07-08T13:37:12Z

I'll investigate it after I have #167 resolved, unless you'd like to start working on it now yourself.

carson-katri · 2020-07-08T15:55:07Z

I can try but I’m not sure where I’d start 🤔

carson-katri · 2020-07-08T15:58:57Z

I got the includes to resolve by adding the c++/v1 as an include directory, but their #include_next couldn’t be found.

MaxDesiatov · 2020-07-08T16:15:49Z

This is probably related to issues like WebAssembly/wasi-sdk#93, but that one was created just for a single header, while I think there are multiple headers like that. If we can make sure that the runtime bits of that OpenCombine PR compile with the standard WASI SDK, we can make similar (or same) adjustments in our SwiftWasm build and then reproduce successful builds with SwiftPM. This is the line that triggers the LLVM headers install rule, so it would probably involve compiling WASI SDK from scratch with changes applied to appropriate CMake files in LLVM/Clang.

Just want to prepare you that working on the SwiftWasm and LLVM/Clang toolchains is a tedious process that requires a ton of disk space (dozens of gigs) and very preferably the most powerful hardware you can get to compile it in reasonable time. There is the LLVM "Getting Started" guide and the upstream "Development tips" doc that I highly recommend to check out.

I would probably start it with creating a new swift-source directory, and cloning the main SwiftWasm repository in it, then triggering the ./swift/utils/webassembly/ci.sh build script from swift-source, also make sure you have selected latest Xcode 12 beta with xcode-select. After the whole build process completes (half an hour to an hour on my 6-core MacBook Pro 15-inch, 2018) you can navigate to the LLVM build directory. It's located at build/Ninja-ReleaseAssert/llvm-macosx-x86_64 and you can trigger ninja from there after you make changes in LLVM's or Clang's CMake files. Ninja then should pick up the changes and rebuild just the bits you've changed. After these incremental rebuilds succeed you can re-run the ci.sh script again, which will recreate the toolchain archive that you'll find in the swift-source directory under the name swift-wasm-DEVELOPMENT-SNAPSHOT-osx.tar.gz.

I haven't looked into it deeply enough, but it's quite possible you'll have to compile your own WASI SDK and then integrate that into our builds. We have our own fork repository for that, but it's slighly outdated, which could cause some problems too 😔

That's basically the intro to the toolchain development in my own words, I'm sure I unintentionally missed some parts, but I don't know what's your experience with the custom toolchain builds. Please let me know if you stumble upon any problems, and it's totally fine to abandon this to focus on other more interesting tasks in the meantime 🙂

MaxDesiatov · 2020-07-09T17:18:12Z

Another totally hacky solution I'd like to try is to modify the headers locally so that it compiles, then either cobbling together an installable SDK from that manually or injecting fixed headers into the toolchain build script. After that works we can look into the LLVM/Clang headers build steps in more details.

MaxDesiatov · 2020-07-09T19:03:11Z

I think I've got it built, haven't verified if it works yet though. Required a couple of changes in both OpenCombine and WASI sysroot, PRs incoming...

This is a first attempt, still not hooked in the reconciler and requires a custom toolchain.

ddddxxx · 2020-07-10T09:02:32Z

Do you consider using CombineX which support ObservableObject and written in pure swift?

MaxDesiatov · 2020-07-10T09:08:42Z

Sorry, I don't think so. OpenCombine supports ObservableObject too and I personally don't care which language it's written in as long as it works. I don't think that migrating to a different framework for no clear reason in the middle of the development process is worth the effort for us. Additionally, as a co-maintainer of OpenCombine, it's much easier for me to collaborate on the project.

ddddxxx · 2020-07-10T09:34:10Z

Yes, I was carefully not suggesting CombineX as you, as the library author, are co-maintainer of OpenCombine (and I'm co-maintainer of CombineX). But I see many technical disadvantages of OpenCombine:

Carton have to bundle OpenCombine binary even it target on macOS 10.15.
OpenCombine requires custom compiler flags.
OpenCombine misses functionality, including ObservableObject.

Anyway, you're the owner and it's all up to you. (And nothing stops you from being a co-maintainer of CombineX)

EDIT: So OpenCombine now supports ObservableObject? That's good.

MaxDesiatov · 2020-07-10T09:40:09Z

Sure, thank you for the suggestions. I'd have to disagree on those points though, except maybe the use of OpenCombine on macOS 10.15 (which I hope we can fix in the near future). There's no requirement for custom compiler flags whatsoever, and as I mentioned above, ObservableObject is not missing, it's being used here in this PR already. Likewise, I'd appreciate addressing your feedback in pull requests and issues of OpenCombine so that we can collaborate in a constructive manner.

carson-katri · 2020-07-10T13:16:37Z

What changes did you have to make to the toolchain for it to work? (any relevant commits?)

MaxDesiatov · 2020-07-10T14:04:22Z

Sorry, no commits yet, but I think you'll be able to reproduce it if you swap ~/.swiftenv/versions/wasm-DEVELOPMENT-SNAPSHOT-2020-07-07-a/usr/share/wasi-sysroot/include/c++/v1/module.modulemap (update the path for your appropriate SDK installation) with this one uncompressed
module.modulemap.zip

It only removes unsupported headers like setjmp and signal, as far as I remember.

I hope to submit a PR that integrates this change in the toolchain builds ASAP.

MaxDesiatov · 2020-07-10T14:09:59Z

@ddddxxx sorry if my response sounded confrontational, I wish I phrased it in a better way. I hope we all can collaborate and improve the ecosystem together without duplicating our work. I'm not sure if there's an easy way out of this unless either OpenCombine or CombineX decide to merge their code into another one, or maybe we could create a separate new project to join our efforts, and I'm not the person to make that decision. Anyway, I think this is offtopic and not very related to Tokamak itself, so I hope we can continue this discussion, if you'd like to, in a more suitable setting outside of Tokamak project issues.

ddddxxx · 2020-07-11T11:35:18Z

@MaxDesiatov It's not confrontational. I'm also curious how do you fix OpenCombine on macOS 10.15, in OpenCombine or Carton? Because I faced the same problem. Should we create an issue in OpenCombine or Carton?

[off-topic]

I feel the division of the community and ecosystem is inevitable. even I did my best to make CX and my libraries also support OpenCombine. Now I hope Apple open source their Combine and solve this once for all.

Update WASI SDK to fix C++ setjmp/signal headers Fixes the issue #595 with C++ packages for the development snapshots made from the `swiftwasm` branch. Previously C++ dependencies couldn't be built as default Clang headers weren't tailored to support WASI, WebAssembly/wasi-sdk#93 being one of the examples. This should also unblock TokamakUI/Tokamak#170.

ie-ahm-robox · 2020-07-16T20:27:57Z

Sources/TokamakCore/Environment/EnvironmentObject.swift

+  func subscribe(_ closure: @escaping () -> ())
+}
+
+@propertyWrapper public struct EnvironmentObject<ObjectType>: EnvironmentReader, EnvironmentWriter where ObjectType: ObservableObject {


🚫 Line should be 100 characters or less: currently 135 characters (line_length)

ie-ahm-robox · 2020-07-16T20:27:58Z

	Fails
🚫	Sources/TokamakCore/Environment/EnvironmentObject.swift#L27 - Line should be 100 characters or less: currently 121 characters (`line_length`)

Generated by 🚫 Danger Swift against 5f8f02e

carson-katri · 2020-07-16T22:14:24Z

I believe the Environment injection issues are resolved. Also EnvironmentObject is implemented 🎉

j-f1

Works great when merged into my navigation branch!

Sources/TokamakCore/MountedViews/MountedView.swift

Sources/TokamakDOM/Views/SecureField.swift

Sources/TokamakDemo/main.swift

MaxDesiatov

Awesome 🙌

Initial EnvironmentObject impl

ba17ce2

carson-katri marked this pull request as draft July 7, 2020 23:33

carson-katri added the SwiftUI compatibility Tokamak API differences with SwiftUI label Jul 7, 2020

ie-ahm-robox reviewed Jul 7, 2020

View reviewed changes

MaxDesiatov added 2 commits July 9, 2020 20:24

Pull observable-object branch of OpenCombine

2505ca9

Add ObservedObject implementation

3a41603

This is a first attempt, still not hooked in the reconciler and requires a custom toolchain.

Setup subscriptions in the reconciler

b9da18a

This was referenced Jul 12, 2020

Update WASI SDK to fix C++ setjmp/signal headers swiftwasm/swift#1444

Merged

Use Combine instead of OpenCombine where possible swiftwasm/carton#39

Merged

MaxDesiatov and others added 3 commits July 14, 2020 20:52

Update .swift-version to the latest snapshot

7fd940e

Merge observed-object

a1ada48

Resolve conflicts

5f8f02e

ie-ahm-robox reviewed Jul 16, 2020

View reviewed changes

carson-katri added 2 commits July 16, 2020 18:09

Resolve environment injection issues

5f0b374

Fix demo

1187772

carson-katri marked this pull request as ready for review July 16, 2020 22:13

carson-katri requested review from ie-ahm-robox, MaxDesiatov and j-f1 July 16, 2020 22:28

j-f1 removed the request for review from ie-ahm-robox July 16, 2020 23:43

j-f1 previously approved these changes Jul 16, 2020

View reviewed changes

MaxDesiatov reviewed Jul 17, 2020

View reviewed changes

Sources/TokamakCore/MountedViews/MountedView.swift Outdated Show resolved Hide resolved

Sources/TokamakDOM/Views/SecureField.swift Outdated Show resolved Hide resolved

Sources/TokamakDemo/main.swift Outdated Show resolved Hide resolved

Formatting fixes

aa4d33d

carson-katri dismissed j-f1’s stale review via aa4d33d July 17, 2020 11:51

carson-katri requested a review from MaxDesiatov July 17, 2020 11:52

MaxDesiatov approved these changes Jul 17, 2020

View reviewed changes

carson-katri merged commit 93927f4 into main Jul 17, 2020

carson-katri deleted the environmentobject branch July 17, 2020 11:54

This was referenced Jul 17, 2020

Add Toggle implementation #159

Merged

Add ColorScheme environment #136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add @EnvironmentObject #170

Add @EnvironmentObject #170

carson-katri commented Jul 7, 2020

ie-ahm-robox commented Jul 7, 2020

ie-ahm-robox Jul 7, 2020

MaxDesiatov commented Jul 7, 2020 •

edited

Loading

MaxDesiatov commented Jul 7, 2020

carson-katri commented Jul 7, 2020

MaxDesiatov commented Jul 7, 2020

MaxDesiatov commented Jul 7, 2020 •

edited

Loading

carson-katri commented Jul 8, 2020

MaxDesiatov commented Jul 8, 2020

carson-katri commented Jul 8, 2020

MaxDesiatov commented Jul 8, 2020

carson-katri commented Jul 8, 2020

carson-katri commented Jul 8, 2020

MaxDesiatov commented Jul 8, 2020 •

edited

Loading

MaxDesiatov commented Jul 9, 2020 •

edited

Loading

MaxDesiatov commented Jul 9, 2020

ddddxxx commented Jul 10, 2020

MaxDesiatov commented Jul 10, 2020

ddddxxx commented Jul 10, 2020 •

edited

Loading

MaxDesiatov commented Jul 10, 2020 •

edited

Loading

carson-katri commented Jul 10, 2020 •

edited

Loading

MaxDesiatov commented Jul 10, 2020

MaxDesiatov commented Jul 10, 2020 •

edited

Loading

ddddxxx commented Jul 11, 2020

ie-ahm-robox Jul 16, 2020

ie-ahm-robox commented Jul 16, 2020

carson-katri commented Jul 16, 2020

j-f1 left a comment

MaxDesiatov left a comment

Add @EnvironmentObject #170

Add @EnvironmentObject #170

Conversation

carson-katri commented Jul 7, 2020

ie-ahm-robox commented Jul 7, 2020

ie-ahm-robox Jul 7, 2020

Choose a reason for hiding this comment

MaxDesiatov commented Jul 7, 2020 • edited Loading

MaxDesiatov commented Jul 7, 2020

carson-katri commented Jul 7, 2020

MaxDesiatov commented Jul 7, 2020

MaxDesiatov commented Jul 7, 2020 • edited Loading

carson-katri commented Jul 8, 2020

MaxDesiatov commented Jul 8, 2020

carson-katri commented Jul 8, 2020

MaxDesiatov commented Jul 8, 2020

carson-katri commented Jul 8, 2020

carson-katri commented Jul 8, 2020

MaxDesiatov commented Jul 8, 2020 • edited Loading

MaxDesiatov commented Jul 9, 2020 • edited Loading

MaxDesiatov commented Jul 9, 2020

ddddxxx commented Jul 10, 2020

MaxDesiatov commented Jul 10, 2020

ddddxxx commented Jul 10, 2020 • edited Loading

MaxDesiatov commented Jul 10, 2020 • edited Loading

carson-katri commented Jul 10, 2020 • edited Loading

MaxDesiatov commented Jul 10, 2020

MaxDesiatov commented Jul 10, 2020 • edited Loading

ddddxxx commented Jul 11, 2020

ie-ahm-robox Jul 16, 2020

Choose a reason for hiding this comment

ie-ahm-robox commented Jul 16, 2020

carson-katri commented Jul 16, 2020

j-f1 left a comment

Choose a reason for hiding this comment

MaxDesiatov left a comment

Choose a reason for hiding this comment

MaxDesiatov commented Jul 7, 2020 •

edited

Loading

MaxDesiatov commented Jul 7, 2020 •

edited

Loading

MaxDesiatov commented Jul 8, 2020 •

edited

Loading

MaxDesiatov commented Jul 9, 2020 •

edited

Loading

ddddxxx commented Jul 10, 2020 •

edited

Loading

MaxDesiatov commented Jul 10, 2020 •

edited

Loading

carson-katri commented Jul 10, 2020 •

edited

Loading

MaxDesiatov commented Jul 10, 2020 •

edited

Loading