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

Implementation for ObservableObject with Mirror #201

Merged
merged 11 commits into from
Jan 25, 2021
Merged

Implementation for ObservableObject with Mirror #201

merged 11 commits into from
Jan 25, 2021

Conversation

kateinoigakukun
Copy link
Contributor

@kateinoigakukun kateinoigakukun commented Dec 22, 2020 •
edited
Loading

Ported from @MaxDesiatov and @broadwaylamb's work and stripped wickwirew/Runtime dependency and unsafe operations.
This implementation only depends on Mirror.

@codecov
Copy link

codecov bot commented Dec 22, 2020 •
edited
Loading

Codecov Report

Merging #201 (d32eec6) into master (1fbf688) will increase coverage by 0.31%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   97.30%   97.61%   +0.31%     
==========================================
  Files          98       98              
  Lines        6649     6676      +27     
==========================================
+ Hits         6470     6517      +47     
+ Misses        179      159      -20
Impacted Files Coverage Δ
Sources/OpenCombine/ObservableObject.swift 97.67% <100.00%> (+7.19%) ⬆️
Sources/OpenCombine/Published.swift 95.91% <100.00%> (+18.14%) ⬆️
Sources/OpenCombine/Helpers/PublishedSubject.swift 97.50% <0.00%> (+6.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fbf688...d32eec6. Read the comment docs.

@OpenCombineBot
Copy link

OpenCombineBot commented Dec 22, 2020 •
edited
Loading

LGTM

Generated by 🚫 Danger Swift against d32eec6

MaxDesiatov
MaxDesiatov requested changes Dec 22, 2020
View reviewed changes
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

There are a few more tests in the original PR that could be added here as well.

@MaxDesiatov MaxDesiatov added the missing functionality This functionality is present in Apple's Combine but we don't have it yet label Dec 22, 2020
MaxDesiatov
MaxDesiatov approved these changes Dec 22, 2020
View reviewed changes
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

👏

@MaxDesiatov MaxDesiatov mentioned this pull request Jan 11, 2021
Merged
@MaxDesiatov
Copy link
Collaborator

Hi @broadwaylamb, would you have a moment to look at this? The lack of working ObservableObject is the only major difference between this repo and our SwiftWasm fork, would be amazing to work with you on getting this reviewed and merge if possible.

broadwaylamb
broadwaylamb reviewed Jan 11, 2021
View reviewed changes
Copy link
Member

@broadwaylamb broadwaylamb left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Left some comments, overall this looks very good as a temporary solution, until I come up with a proper implementation which will also work with classes that conform to CustomReflectable.

Sources/OpenCombine/Published.swift Outdated Show resolved Hide resolved
Sources/OpenCombine/Published.swift Outdated Show resolved Hide resolved
Tests/OpenCombineTests/ObservableObjectTests.swift Outdated Show resolved Hide resolved
Tests/OpenCombineTests/ObservableObjectTests.swift Outdated
var counter = 0

// A bug in Combine (FB7471594). It should not crash. Why would it crash?
assertCrashesOnDarwin {
Copy link
Member

Choose a reason for hiding this comment

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

This bug has been fixed, it doesn't crash anymore. Let's remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it seems this bug is not fixed on macOS 10.15.7. Which version are you using?

Copy link
Member

@broadwaylamb broadwaylamb Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The latest iOS. Combine on macOS is always a little bit behind its iOS version.
I case of Catalina, it is quite behind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should we run this test suit on only iOS?

Copy link
Member

Choose a reason for hiding this comment

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

When testing against Combine — yes. But this applies to all the tests, not just this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I addressed 👍

Tests/OpenCombineTests/ObservableObjectTests.swift Outdated Show resolved Hide resolved
Tests/OpenCombineTests/Helpers/AssertCrashes.swift Outdated Show resolved Hide resolved
@MaxDesiatov MaxDesiatov mentioned this pull request Jan 11, 2021
Merged
broadwaylamb
broadwaylamb reviewed Jan 13, 2021
View reviewed changes
Tests/OpenCombineTests/ObservableObjectTests.swift Outdated
Comment on lines 215 to 217
// A bug in Combine (FB7471594). This has been fixed on iOS.
// But not deployed on other Darwin OS yet
#if !OPENCOMBINE_COMPATIBILITY_TEST || os(iOS)
Copy link
Member

Choose a reason for hiding this comment

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

No, this condition is not needed. What I mean is that one should make sure to only run compatibility tests against the most recent version of Combine (since that's what we aim to be compatible with). It so happens that the most recent version of Combine is usually vendored in the newest iOS version (not even the newest macOS version, and certainly not Catalina).

Tests should not be disabled based on that.

Just FYI, you can run the compatibility tests in the iOS simulator, this way they should pass. This is exactly what we do on our CI build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kateinoigakukun kateinoigakukun force-pushed the katei/observable-object branch from bdfe91b to d32eec6 Compare January 13, 2021 12:17
broadwaylamb
broadwaylamb approved these changes Jan 25, 2021
View reviewed changes
Copy link
Member

@broadwaylamb broadwaylamb left a comment

Choose a reason for hiding this comment

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

Thank you!

@broadwaylamb broadwaylamb merged commit beb38de into OpenCombine:master Jan 25, 2021
@MaxDesiatov MaxDesiatov mentioned this pull request Jan 30, 2021
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov approved these changes

@broadwaylamb broadwaylamb broadwaylamb approved these changes

Assignees
No one assigned
Labels
missing functionality This functionality is present in Apple's Combine but we don't have it yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kateinoigakukun @OpenCombineBot @MaxDesiatov @broadwaylamb