Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add @ObservedObject implementation #171

Merged
merged 11 commits into from
Jul 16, 2020
Merged

Add @ObservedObject implementation #171

merged 11 commits into from
Jul 16, 2020

Conversation

MaxDesiatov
Copy link
Collaborator

@MaxDesiatov MaxDesiatov commented Jul 9, 2020 •
edited
Loading

This pulls a fork of OpenCombine that can be compiled with the same SwiftWasm snapshot we use in main.

The only caveat is that this doesn't work for ObservableObjects that are subclasses of other classes with @Published properties. This issue needs to be fixed in the Runtime library fork. Since this is a rare case, and fixing it wouldn't change this @ObservedObject implementation, I think it's ready for review as is.

@MaxDesiatov
Pull observable-object branch of OpenCombine
2505ca9
@MaxDesiatov
Add ObservedObject implementation
3a41603 
This is a first attempt, still not hooked in the reconciler and requires a custom toolchain.
@MaxDesiatov MaxDesiatov added the SwiftUI compatibility Tokamak API differences with SwiftUI label Jul 9, 2020
@ie-ahm-robox
Copy link
Collaborator

Fails
🚫

Sources/TokamakDOM/Shapes/Path.swift#L57 - Line should be 100 characters or less: currently 113 characters (line_length)

Generated by 🚫 Danger Swift against 3a41603

ie-ahm-robox
ie-ahm-robox reviewed Jul 9, 2020
View reviewed changes
Sources/TokamakDOM/Shapes/Path.swift Outdated
@@ -50,7 +50,7 @@ extension Path: ViewDeferredToRenderer {
"rx": "\(roundedRect.cornerSize.width)",
"ry": "\(roundedRect.style == .continuous ? roundedRect.cornerSize.width : roundedRect.cornerSize.height)",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MaxDesiatov MaxDesiatov changed the title Add ObservedObject implementation Add @ObservedObject implementation Jul 10, 2020
@ie-ahm-robox
Copy link
Collaborator

Fails
🚫

Sources/TokamakDOM/Shapes/Path.swift#L57 - Line should be 100 characters or less: currently 113 characters (line_length)

Generated by 🚫 Danger Swift against b9da18a

ie-ahm-robox
ie-ahm-robox reviewed Jul 10, 2020
View reviewed changes
Sources/TokamakDOM/Shapes/Path.swift Outdated
@@ -50,7 +50,7 @@ extension Path: ViewDeferredToRenderer {
"rx": "\(roundedRect.cornerSize.width)",
"ry": "\(roundedRect.style == .continuous ? roundedRect.cornerSize.width : roundedRect.cornerSize.height)",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@carson-katri
Copy link
Member

carson-katri commented Jul 15, 2020 •
edited
Loading

What is left in OpenCombine for this to be working?

@MaxDesiatov
Copy link
Collaborator Author

MaxDesiatov commented Jul 15, 2020 •
edited
Loading

In my current investigation, I'm not entirely convinced it's a problem in OpenCombine (at least not in its high-level code), this is either a problem in SwiftWasm runtime, or the way that COpenCombineHelpers processes runtime metadata when looking for the @Published property. I also tried to swap that C++ implementation to the Runtime library, but got similar errors. I've tried running the Runtime library test suite, which currently fails with classes (GetSetClassTests test cases), I don't remember seeing those test failures a couple of months ago. I've got carton test implemented in the main branch of carton and I'm currently working on fixing those test failures. If the OpenCombine issues persist after those tests are fixed, I think we should isolate that failure and add as a test case to avoid breakage in the future.

@MaxDesiatov
Copy link
Collaborator Author

MaxDesiatov commented Jul 15, 2020 •
edited
Loading

TWIMC, I'm currently looking at the type metadata docs, seems like both Runtime library and COpenCombineHelpers have problems particulalrly with classes metadata, even when classes aren't generic. These issues are reproducible only with SwiftWasm, I'm not sure if this is because of some 64-bit platform assumptions in their code, or due to some issue we have in SwiftWasm. Since the rest of the things in SwiftWasm work pretty well, at least to my knowledge, I think it's more about the runtime introspection code in those libraries, but it's not that easy to debug...

@MaxDesiatov
Copy link
Collaborator Author

MaxDesiatov commented Jul 15, 2020 •
edited
Loading

  • On platforms which support Objective-C interoperability, two words are
    reserved for use by the Objective-C runtime at offset 2 and offset
    3    ; on other platforms, nothing is reserved.
  • On platforms which support Objective-C interoperability, the rodata
    pointer     is stored at offset 4; on other platforms, it is not present.

That could be it, but I thought that both of those libraries work well on Linux, which doesn't have Objective-C interop 🤔

@MaxDesiatov
Copy link
Collaborator Author

MaxDesiatov commented Jul 15, 2020 •
edited
Loading

Given that swiftlang/swift#32374 was merged only recently, they might have changed metadata layout on non-Apple platforms recently as they don't need to maintain ABI stability on those platforms. This would explain why the Runtime test suite was passing back in March/April for me with old SwiftWasm snapshots...

@MaxDesiatov MaxDesiatov requested review from carson-katri and j-f1 July 16, 2020 12:52
@MaxDesiatov
Copy link
Collaborator Author

This no longer needs any custom or new toolchains, the original one works fine 🙂

@MaxDesiatov MaxDesiatov marked this pull request as ready for review July 16, 2020 12:53
@MaxDesiatov
Copy link
Collaborator Author

MaxDesiatov commented Jul 16, 2020 •
edited
Loading

@carson-katri @j-f1 after this one's merged I think I'm going to tag Tokamak 0.2.0 as is, so then carton init template introduced in swiftwasm/carton#54 could specify 0.2.0 version instead of the main branch. I'd prefer users specify a specific version in their package manifests to avoid unexpected breakage. Additionally, if any 3rd-party library depending on Tokamak appears, if both user's application and that library depend on specific Tokamak version, it's much easier to resolve the dependency tree, but harder if either of those depend on a branch.

carson-katri
carson-katri previously approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@carson-katri carson-katri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MaxDesiatov MaxDesiatov requested a review from carson-katri July 16, 2020 18:21
@MaxDesiatov
Copy link
Collaborator Author

Conflicts resolved now.

@MaxDesiatov MaxDesiatov merged commit ffa686c into main Jul 16, 2020
@MaxDesiatov MaxDesiatov deleted the observed-object branch July 16, 2020 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ie-ahm-robox ie-ahm-robox ie-ahm-robox left review comments

@j-f1 j-f1 j-f1 approved these changes

@carson-katri carson-katri Awaiting requested review from carson-katri

Assignees
No one assigned
Labels
SwiftUI compatibility Tokamak API differences with SwiftUI
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MaxDesiatov @ie-ahm-robox @carson-katri @j-f1