search

TwilioDevEd / twiliochat-swift

Swift implementation of Twilio Chat
https://www.twilio.com/docs/tutorials/walkthrough/ip-chat/ios/swift
MIT License
46 stars 18 forks source link

[TwilioChatClient] Swift interface looks sketchy at best #20

Open tmspzz opened 7 years ago

tmspzz commented 7 years ago

Hi,

we are currently considering Twilio for a large chat application but the TwilioChatClient Swift interface looks sketchy at best.

Consider these lines from TCHChannel:

    /** Optional channel delegate */
    weak open var delegate: TCHChannelDelegate!

Am I looking at production code? Is there a newer version of the SDK I should be looking at? I am Currently on 0.16.1

Is this the right forum where to ask questions?

otymartin commented 7 years ago

@blender https://www.twilio.com/docs/api/chat/changelogs/ios

tmspzz commented 7 years ago

@otymartin Thanks! However does not seem like this has changed much (looking at 1.0.4)

/** Optional channel delegate.  Upon setting the delegate, you will receive the current channel synchronization status by delegate method. */
    weak open var delegate: TCHChannelDelegate!
otymartin commented 7 years ago

@blender Im failing to understand your problem with that delegate declaration? weak open car delegate: TCHCHannelDelegate!?

tmspzz commented 7 years ago

Since the delegate is weak it can be nil at any moment. Accessing it by channel.delegate when this is declared implicitly unwrapped can cause a segmentation fault at any moment (if in fact it happens to be nil).

It is good practice (and much safer) to declare delegates as weak and optional. 

 weak open var delegate: TCHChannelDelegate?

I am going around this by defining in an extension 

extension TCHChannel: ChatChannelWithDelegate {

    public weak var optionalDelegate: TCHChannelDelegate? {

        guard let delegate = self.delegate else { return nil }

        return delegate
    }
 }

Poking around this gives me very little confidence is the status of the SDK and the abstractions that one can build around it.

otymartin commented 7 years ago

@blender Good find, it completely went over my head. @rbeiter

tmspzz commented 7 years ago

I am guessing it's just a matter of adding proper nullability annotations to the objc sdk. Swift has been around for quite some time now.

If anyone from Twilio reads this please let me know if this is the right forum or if I should be discussing this elsewhere. Thanks.

rbeiter commented 7 years ago

Hi @blender - I'm on the team taking care of the iOS Chat SDK's. This is indeed related to not having nullability hints in the Obj-C interfaces and addressing this is already in process for 2.x which is coming soon. We'll update this issue when 2.0 is released. Let us know here or through our support portal (https://support.twilio.com/) if we can assist further. Thanks!

tmspzz commented 7 years ago

@rbeiter Thank you very much. Looking forward to the changes.

tmspzz commented 7 years ago

@rbeiter TCHMessage timeStampAsDate is nil for this string 2017-08-17T15:34:30.513Z

Since timeStampAsDate is Date! this causes a crash. Twilio chat 1.0.7.

See proper ISO date parsing here: https://github.com/cheeyi/ProjectSora/blob/master/Pods/AFDateHelper/AFDateHelper/AFDateExtension.swift