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.

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

Create ReactiveSwift.framework #3137

Merged
merged 28 commits into from Sep 11, 2016
Merged

Create ReactiveSwift.framework #3137

merged 28 commits into from Sep 11, 2016

Conversation

mdiep
Copy link
Contributor

@mdiep mdiep commented Aug 16, 2016

Supersedes #2980.

This introduces ReactiveSwift.framework. I've done this as a PR here to allow the changes to be reviewed. Once merged, I will clone this repo, remove RAC, and push to ReactiveSwift. I will then open a PR to update this repo to get ReactiveSwift as a submodule.

Of note: I first duplicated the ReactiveCocoa files and then deleted files from each framework.

@@ -11,7 +11,7 @@ script:
xcode_workspace: ReactiveCocoa.xcworkspace
matrix:
include:
- xcode_scheme: ReactiveCocoa-Mac
- xcode_scheme: ReactiveCocoa-macOS
Copy link
Member

Choose a reason for hiding this comment

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

👍

@mdiep
Copy link
Contributor Author

mdiep commented Aug 18, 2016

Feedback is addressed.

Tests don't pass on CI because Travis was updating to ß6.

# Conflicts:
#	ReactiveCocoa/Swift/Lifetime.swift
#	ReactiveSwiftTests/PropertySpec.swift
let package = Package(
name: "ReactiveCocoa",
name: "ReactiveSwift",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we have two packages here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I'm not sure if a package for ReactiveCocoa is useful
  2. I couldn't find how to make write a Package.swift to make multiple packages or use intra-repository dependencies

If this is something we want, I think we should add it back when ReactiveSwift is moved to its own repo.

@ikesyo
Copy link
Member

ikesyo commented Aug 23, 2016

SwiftPM build is broken 😞 https://travis-ci.org/ReactiveCocoa/ReactiveCocoa/jobs/154453152

This symlink should be changed.

@mdiep
Copy link
Contributor Author

mdiep commented Aug 24, 2016

This is ready to go! 🎉

The carthage build is having some trouble due to timeouts on Travis, but this should get better as we split up the repos.

@mdiep
Copy link
Contributor Author

mdiep commented Aug 27, 2016

@ReactiveCocoa/reactivecocoa Anyone want to review this? 😄

@NachoSoto
Copy link
Member

I have this in my TODO list!

@andersio
Copy link
Member

Should we consider re-exporting ReactiveSwift in ReactiveCocoa?
https://github.com/apple/swift/blob/master/docs/Modules.rst#id13

@mdiep
Copy link
Contributor Author

mdiep commented Aug 30, 2016

Should we consider re-exporting ReactiveSwift in ReactiveCocoa?

I think we should wait until it's a stable feature.

# Conflicts:
#	ReactiveCocoa.xcodeproj/project.pbxproj
#	ReactiveCocoa/Swift/Lifetime.swift
#	ReactiveSwiftTests/PropertySpec.swift
# Conflicts:
#	Carthage/Checkouts/Nimble
#	Carthage/Checkouts/Quick
@mdiep
Copy link
Contributor Author

mdiep commented Sep 5, 2016

I'll self-merge this at the end of the week if no one has reviewed it before then. 😄

}
}
}

#if os(macOS) || os(iOS) || os(tvOS) || os(watchOS)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary, since Objective-C is present on all Darwin platforms.

@andersio
Copy link
Member

andersio commented Sep 6, 2016

May you reorder the schemes please?

By the way, Lifetime.swift in ReactiveCocoa could be renamed as NSObject+Lifetime.swift, since it carries just an extension. The organisation in the Xcode project might be flattened too.

@@ -1,6 +1,6 @@
import Quick
import Nimble
import ReactiveCocoa
import ReactiveSwift
import Result

final class LifetimeSpec: QuickSpec {
Copy link
Member

@andersio andersio Sep 6, 2016

Choose a reason for hiding this comment

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

The description perhaps should be changed, and I wonder if the test cases should be duplicated for NSObject.rac_lifetime in ReactiveCocoa.

@andersio
Copy link
Member

andersio commented Sep 6, 2016

Scheduler.swift carries Darwin-specific code (OSAtomic*).

@mdiep
Copy link
Contributor Author

mdiep commented Sep 7, 2016

May you reorder the schemes please?

Schemes ordering is a local thing AFAIK. It's not affected by anything here. (All my schemes are ordered correctly.)

I wonder if the test cases should be duplicated for NSObject.rac_lifetime in ReactiveCocoa.

Probably, but I think that's beyond the scope of this PR.

Scheduler.swift carries Darwin-specific code (OSAtomic*).

We'll have to fix that post-split.

isa = PBXSourcesBuildPhase;
buildActionMask = 2147483647;
files = (
D08C54B41A69A2AF00AD8286 /* Signal.swift in Sour
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this setting to favor the .xcconfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which settings? GitHub seems to have misplaced your comment.

@NachoSoto
Copy link
Member

NachoSoto commented Sep 7, 2016

Just a couple of comments, but I think this is ready!
Thanks for the hard work 👍

@mdiep
Copy link
Contributor Author

mdiep commented Sep 10, 2016

Okay, I'm planning to merge this in ~24 hours. (Unless anyone speaks up.) At that point, I'll push a copy to https://github.com/ReactiveCocoa/ReactiveSwift/, make it public, and open a PR to remove the RAC bits. I'll probably merge it right away.

Then I'll open a PR to RAC to make it depend on ReactiveSwift and continue by going through this same process with ReactiveObjC and ReactiveObjCBridge.

@mdiep mdiep merged commit d6f6f4f into master Sep 11, 2016
@mdiep mdiep deleted the reactive-swift branch September 11, 2016 01:04
@mdiep
Copy link
Contributor Author

mdiep commented Sep 11, 2016

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants