parley-messaging / ios-library

Parley iOS app library
MIT License
2 stars 2 forks source link

Implement ParleyNetwork library with ParleyNetworkSession so you can replace Alamofire with your own network stack #58

Closed mat1th closed 4 months ago

mat1th commented 5 months ago

Implemented in this pr:

Open tasks:

mat1th commented 5 months ago

Thanks for this PR @mat1th ! We'll need to review this though, bigger change overall. With a quick look this looks like a separtion we wanna go on with. I've added a few comments while quickly looking at it.

We will respond in more detail when we also looked into it 👍

Thank you, I've resolved or replayed your comments.

mat1th commented 5 months ago

Looks good! We'll test some more though.

Few more comments during the review; also: Seems like we don't need fileprivate inside the Parley class. private should work fine since the extension is in the same file, right?

Meaning that:

* secret

* pushToken

* pushType

* pushEnabled

* referrer

* userAuthorization

* userAdditionalInformation

* configure (the one outside the extension)

* reconfigure

* clearChat

* registerDevice

* handleMessage

* handleEvent

Can remain private instead of fileprivate, correct?

(note that some of these weren't due to your changes, they werent private in the previous version either we see)

I've marked them as private now. Could you recheck because I've also re-based the branch with develop.

mat1th commented 4 months ago

Hi @alexkok what is the status on this pr? Are there things that needs to be done on my side?

alexkok commented 4 months ago

Hi @mat1th , thanks! Your PR looks good, we also tested it, no changes needed from your side currently.

FYI: we'll additionally do some more changes for the 4.0.0 release, since it's a major one. Concerning the network separation (this PR), we don't expect changes on the calling side of Parley. Might there be any, they won't be big.

We're on it, and will merge this PR soon to the update/4.0.0 branch, which will be receiving some more changes for the 4.0.0 release.

mat1th commented 4 months ago

Thanks for the update. Great that you are working on more improvements for the 4.0 release.

alexkok commented 4 months ago

This one has now been merged to the update/4.0.0 branch 🚀

4.0.0 will get some changes though

alexkok commented 4 months ago

@mat1th coming back to this one, with 4.0.0 we actually did a few adjustments to the calling side of this.

Not sure if you've seen them already. They are not big, but wanted to let you know in advance before we release 4.0.0:

You can check with the current update/4.0.0 branch, only the latter change (the Parley prefix) is not in there yet. That will be tomorrow.