Fixed Xcode 10 Support #221

cocojoe · 2018-09-13T20:00:53Z

Updated Loggable Protocol & Extension to workaround 4.2 compiler issue
Removed test due to potential issue in Nimble
Updated Dependencies
Added support for compiling as 4.2

Removed test due to potential issue in Nimble Updated Dependencies

guykogus · 2018-09-14T09:06:29Z

Auth0/SafariWebAuth.swift

@@ -23,6 +23,12 @@
 import UIKit
 import SafariServices

+#if swift(>=4.2)
+public typealias OpenURLOptionsKey = UIApplication.OpenURLOptionsKey


I don't think setting a public typealias is a good idea as it can conflict with others. Perhaps it should be inside a struct or something, e.g.

struct Auth0Application { #if swift ... #endif }

Would that be possible? That way we can keep backwards compatibility without risking others conflicting, in case they try to use the same trick.

It also doesn't make sense to define it in this class instead of in Auth0.swift, for example.

Thoughts?

It has to be public due to some of the methods that include it as a param type being public. I could prefix it to avoid an unlikely collision e.g. Auth0URLOptionsKey

Auth0.swift doesn't include UIKit as it's used cross platform, UIKit is for iOS and the only areas that use this are in the files that are only included for iOS.

guykogus · 2018-09-14T09:08:10Z

App/ViewController.swift

@@ -35,22 +35,22 @@ class ViewController: UIViewController {
    }

    @IBAction func startOAuth2(_ sender: Any) {
-        var auth0 = Auth0.webAuth()
+        let auth0 = Auth0.webAuth()


If you've made this non-variable you can skip the whole let auth0 = bit and just go straight into

Auth0 .webAuth() .logging...

Yeah good point, the sample App isn't a priority. However, it will look cleaner 👍

guykogus · 2018-09-14T13:45:53Z

Auth0Tests/ResponseSpec.swift

-                let response = Response<AuthenticationError>(data: data, response: http(200), error: nil)
-                expect(try? response.result()).to(beNil())
-            }
+//            it("should fail with invalid JSON") {


Why is this failing?

Well it's a strange error, it expects nil but gets Optional(nil) I feel it's most likely an issue in Quick/Nimble magic while libs mature for Xcode 10.

OK, so I've discovered the issue and it's a weird change in Swift 4.2. Basically if you say

if let foo = Optional<Any>.none as? Any { ... }

it will enter the block, even though foo is nil, because Any represents anything, even an Optional<T>.none. This is different behaviour to older versions of Swift.

In Response.swift the top function json is being called with
if let json: Any = json(data) {
and even though it returns nil it still enters the block because it has accepted that .none is Any.

The solution is to change the json function to this:

func json(_ data: Data?) -> Any? { guard let data = data else { return nil } return try? JSONSerialization.jsonObject(with: data, options: []) }

and then manually cast the 2 usages, i.e.
if let json: [String: Any] = json(data) -> if let json = json(data) as? [String: Any] {
and
if let json: Any = json(data) -> if let json = json(data) {

Minor correction, if let foo = Optional<Any>.none as? Any { won't enter the block. It turns out that this issue is related to generic functions being casted to Any.

This is a change between 4.1 and 4.2? You have some source on this change. Thx

I ran this in a playground on Xcode 10 and Xcode 9.4.1 and got different results!

func bar<T>() -> T? { return Optional<T>.none as? T } if let foo: Any = bar() { print("foo has value \(foo)") // called in Xcode 10 (Swift 4.2) } else { print("foo is nil") // called in Xcode 9 (Swift 4.1) }

Looking at the changelog for Swift, the last change for Swift 4.2 is:

Runtime query of conditional conformances is now implemented. Therefore, a dynamic cast such as value as? P, where the dynamic type of value conditionally conforms to P, will succeed when the conditional requirements are met.

You can read the full details of the proposal here. It's pretty complicated and doesn't mention anything regarding Optional+Any specifically, but I'm guessing that this is the cause.

lbalmaceda · 2018-09-14T13:59:51Z

Auth0/SafariAuthenticationCallback.swift

@@ -40,7 +40,7 @@ class SafariAuthenticationSessionCallback: AuthTransaction {
        self.authSession?.start()
    }

-    func resume(_ url: URL, options: [UIApplicationOpenURLOptionsKey: Any]) -> Bool {
+    func resume(_ url: URL, options: [A0URLOptionsKey: Any]) -> Bool {


I'm not familiar with the typealias you defined below. But you're changing the type of the options parameter. Isn't this a breaking change? Applies to all signature changes

No, it's actually maintaining the type signature depending on the version of Swift being used. Apple have unfortunately broken backwards source compatibility and this is a workaround for it.

It doesn't really affect the signature, it looks different in source but the compiler looks to the typealias to see what type it really is and will then use that. So no changes for existing code.

I will also be testing Lock against this before any release.

I believe the only case this might affect someone is when a developer tries to implement the protocol and the method resume on their own, e.g. mocking in tests

@hzalaz do you feel that's a deal breaker for this change?

@cocojoe for me no, kinda forced by xcode so there is no other way than making a major but imho is not worth a major for this change alone.

lbalmaceda · 2018-09-14T14:01:06Z

Auth0/WebAuth.swift

@@ -22,6 +22,12 @@

 import UIKit

+#if swift(>=4.2)
+public typealias A0URLOptionsKey = UIApplication.OpenURLOptionsKey


Fix indentation

So why it may not seem logically correct due to the if, this is how the editor will auto format it and also passes lint.
This is pre-processor syntax so handled differently.

cocojoe · 2018-09-17T15:08:16Z

#220

Updated Loggable Protocol & Extension to workaround compiler issue

4ef9a20

Removed test due to potential issue in Nimble Updated Dependencies

cocojoe changed the title ~~Updated Loggable Protocol & Extension to workaround compiler issue~~ Updated Loggable Protocol & Extension to workaround 4.2 compiler issue Sep 13, 2018

Added Typealias for 4.2 changes

f27aea7

cocojoe changed the title ~~Updated Loggable Protocol & Extension to workaround 4.2 compiler issue~~ Xcode 10 Update Sep 13, 2018

cocojoe mentioned this pull request Sep 13, 2018

Compiler Issue in Xcode 10 Discussion #220

Closed

OrestNazarewycz previously approved these changes Sep 13, 2018

View reviewed changes

guykogus reviewed Sep 14, 2018

View reviewed changes

Chore Tidy

2bb9962

cocojoe dismissed OrestNazarewycz’s stale review via 2bb9962 September 14, 2018 11:26

guykogus reviewed Sep 14, 2018

View reviewed changes

lbalmaceda reviewed Sep 14, 2018

View reviewed changes

Conditional changes to compliy changes in 4.2 runtime behaviour

d23a46e

hzalaz approved these changes Sep 17, 2018

View reviewed changes

cocojoe merged commit 0665d50 into master Sep 17, 2018

cocojoe deleted the fixed-xcode10 branch September 17, 2018 14:47

cocojoe added this to the vNext milestone Sep 17, 2018

cocojoe added the CH: Fixed label Sep 17, 2018

cocojoe changed the title ~~Xcode 10 Update~~ Fixed Xcode 10 Support Sep 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Xcode 10 Support #221

Fixed Xcode 10 Support #221

cocojoe commented Sep 13, 2018 •

edited

Loading

guykogus Sep 14, 2018

cocojoe Sep 14, 2018

guykogus Sep 14, 2018

cocojoe Sep 14, 2018

guykogus Sep 14, 2018

cocojoe Sep 14, 2018

guykogus Sep 14, 2018

guykogus Sep 14, 2018

cocojoe Sep 14, 2018

guykogus Sep 14, 2018

guykogus Sep 15, 2018

lbalmaceda Sep 14, 2018

guykogus Sep 14, 2018

cocojoe Sep 14, 2018

cocojoe Sep 14, 2018

hzalaz Sep 14, 2018

cocojoe Sep 17, 2018

hzalaz Sep 17, 2018

lbalmaceda Sep 14, 2018

cocojoe Sep 14, 2018

cocojoe commented Sep 17, 2018

Fixed Xcode 10 Support #221

Fixed Xcode 10 Support #221

Conversation

cocojoe commented Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe commented Sep 17, 2018

cocojoe commented Sep 13, 2018 •

edited

Loading