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

Swift 4.1 migration #590

Merged
merged 28 commits into from
May 14, 2018
Merged

Swift 4.1 migration #590

merged 28 commits into from
May 14, 2018

Conversation

NachoSoto
Copy link
Member

@NachoSoto NachoSoto commented Jan 19, 2018

Last updated and tested on: Xcode 9.3 GM.

Checklist

  • Updated CHANGELOG.md.
  • Remove BindingSource workarounds
  • Make Signal.Event and ActionError Equatable
  • .flatMap -> .compactMap
  • Remove no longer necessary Result.materialize extension
  • Wait for Xcode 9.3
  • Update Travis
  • Tag final Result release Can be done separately.
  • Wait for fix to .logEvents regression -> Workaround is implemented, doesn't seem like they'll fix the regression.
  • Maintain Swift 4.0.x compatibility using #if swift(>=4.1)
  • Verify UnsafeMutablePointer deprecation changes (I'm not totally sure about them)

@andersio
Copy link
Member

The where clauses for Error in Property.swift can be removed presumably.

@NachoSoto
Copy link
Member Author

@andersio yup, done!

@NachoSoto
Copy link
Member Author

Tested this on beta 2, tests still passing but the regression is still not fixed.

@NachoSoto NachoSoto force-pushed the swift-4.1 branch 2 times, most recently from c79b683 to b2de826 Compare February 10, 2018 01:57
@NachoSoto
Copy link
Member Author

NachoSoto commented Feb 10, 2018

I can't believe we need this workaround: 5bcb5e7 🤦‍♂️

@sharplet
Copy link
Contributor

@mishagray
Copy link
Contributor

I'm seeing an issue with swift 4.1 compability and this PR:
#601

Basically.. all of my my 'signal.map { Int($0) }` is now resolving to a SignalProducer with a value that is a closure, and when I click on 'map' it resolves to the new map method from #601

Not sure how it behaves in swift 4.0, but swift 4.1 has issues. Not sure if this is just a bug in 9.3 beta 2.

@NachoSoto
Copy link
Member Author

Interesting. Any chance you can provide a small test case for that? I can take a look, and maybe add it to the test suite.

@mishagray
Copy link
Contributor

mishagray commented Feb 12, 2018

	let fetchImage = SignalProducer<UIImage, NoError>(value: UIImage())
	let optionalFetch = fetchImage.map { $0 as UIImage? }

	assert(optionalFetch is SignalProducer<UIImage?, NoError>, "did I get the right type?")


	let optionalFetch1: SignalProducer<UIImage?, NoError>  = fetchImage.map { $0 as UIImage? }
	let optionalFetch2: SignalProducer<(UIImage?) -> UIImage?, NoError>  = fetchImage.map { $0 as UIImage? }

So in swift 4.0 ... optionalFetch will be the expected type SignalProducer<UIImage?, NoError>. In swift 4.1 ... we get SignalProducer<(UIImage?) -> UIImage?, NoError>

Both of the 'optionalFetch1' and 'optionalFetch2' lines compile successfully in swift 4.1

@NachoSoto
Copy link
Member Author

What happens if you do:

let optionalFetch2: SignalProducer<UIImage?, NoError>  = fetchImage.map { $0 as UIImage? }

@mishagray
Copy link
Contributor

Ah... so this isn't NOT related to swift 4.1. I wrote that code above and got the same warning error on assertion line 'Cast from 'SignalProducer<(UIImage?) -> UIImage?, NoError>' to unrelated type 'SignalProducer<UIImage?, NoError>' always fails'

Every line compiles in swift 4.0/swift 4.1. I could just open this as in issue against #601. Since it's not actually realted to 4.1.

I just first saw it in my swift 4.1 branch, but I think it's unrelated to swift 4.1

@NachoSoto
Copy link
Member Author

Ooooh yup, totally, it's related to #601, bad overload resolution :( Can you open an issue for that?
Thanks!

@mbuchetics
Copy link

What's the current status now that Xcode 9.3 and Swift 4.1 are officially released?

@NachoSoto
Copy link
Member Author

I'm away for a month and won't be able to continue working on this, somebody else please pick it up from here? 😊

@mdiep
Copy link
Contributor

mdiep commented Apr 3, 2018

The current ReactiveSwift release works with Xcode 9.3 / Swift 4.1. It just has some warnings.

@gunterhager
Copy link

Interesting. You're right, i've just tested it. It seems to work with Xcode 9.4/Swift 4.1.1 beta, too. Used to fail with older Xcode 9.3 betas.

@andersio andersio self-assigned this Apr 15, 2018
@andersio andersio requested a review from mdiep April 15, 2018 10:52
@andersio
Copy link
Member

@ikesyo any plan to release Result with Swift 4.1 soon?

@andersio andersio added this to the 4.0 milestone Apr 15, 2018
@ikesyo
Copy link
Member

ikesyo commented Apr 27, 2018

@andersio Sorry for the delay, Result 4.0 has been released 🚢

https://github.com/antitypical/Result/releases/tag/4.0.0

@mdiep mdiep changed the title [WIP] Swift 4.1 migration Swift 4.1 migration May 2, 2018
@mdiep
Copy link
Contributor

mdiep commented May 2, 2018

Do we want to maintain Xcode 9.2 / Swift 4.0.1 compatibility? As-is, this breaks compatibility (from changes I made) but retains some code for Swift <4.1. I'm not sure whether we should rip out the shims or add more in.

@andersio
Copy link
Member

andersio commented May 7, 2018

I am for supporting only Swift 4.1 given that we are going to bump the major version anyway.

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

8 participants