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

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

Merged
merged 10 commits into from
Feb 16, 2024
Merged

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

merged 10 commits into from
Feb 16, 2024

Conversation

mat1th
Copy link
Contributor

@mat1th mat1th commented Feb 7, 2024

Implemented in this pr:

  • Split up Parley package to Parley and ParleyNetwork. You can now use Parley with your own http network layer. To do so you can implement ParleyNetworkSession.
  • Move configuring network settings to the Parley.configure function to make the setup order more clear.
  • Rename ParleyNetwork to ParleyNetworkConfig to better represent the what the struct does.
  • Make code more strict by using let instead of var.

Open tasks:

Package.swift Show resolved Hide resolved
Sources/Parley/Data/Models/Agent.swift Show resolved Hide resolved
Sources/Parley/Data/Models/Device.swift Show resolved Hide resolved
Sources/Parley/Parley.swift Outdated Show resolved Hide resolved
@alexkok alexkok self-assigned this Feb 7, 2024
Copy link
Collaborator

@alexkok alexkok left a comment

Choose a reason for hiding this comment

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

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 👍

Sources/Parley/Data/Remotes/RequestCancable.swift Outdated Show resolved Hide resolved
Sources/Parley/Data/Models/Agent.swift Show resolved Hide resolved
Sources/Parley/Data/Models/Device.swift Show resolved Hide resolved
Sources/Parley/Parley.swift Outdated Show resolved Hide resolved
@mat1th
Copy link
Contributor Author

mat1th commented Feb 7, 2024

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 mat1th changed the base branch from master to develop February 7, 2024 16:12
@mat1th mat1th requested a review from alexkok February 8, 2024 13:23
Copy link
Collaborator

@alexkok alexkok left a comment

Choose a reason for hiding this comment

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

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)

Sources/Parley/ParleyNetwork.swift Show resolved Hide resolved
Sources/Parley/ParleyNetwork.swift Outdated Show resolved Hide resolved
@mat1th
Copy link
Contributor Author

mat1th commented Feb 9, 2024

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 mat1th requested a review from alexkok February 9, 2024 13:11
@mat1th
Copy link
Contributor Author

mat1th commented Feb 14, 2024

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

@alexkok
Copy link
Collaborator

alexkok commented Feb 14, 2024

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.

@alexkok alexkok added the ready Fixed and will be published in the next release. label Feb 14, 2024
@mat1th
Copy link
Contributor Author

mat1th commented Feb 14, 2024

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

@alexkok alexkok changed the base branch from develop to update/4.0.0 February 16, 2024 12:17
@alexkok alexkok merged commit 3a42d51 into parley-messaging:update/4.0.0 Feb 16, 2024
@alexkok
Copy link
Collaborator

alexkok commented Feb 16, 2024

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

4.0.0 will get some changes though

@mat1th mat1th deleted the feature/move-parley-networking-to-seperate-target branch February 16, 2024 13:35
@alexkok
Copy link
Collaborator

alexkok commented Mar 13, 2024

@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:

  • The ParleyNetworkSession callback to retrieve an UIImage has been removed, we handle it with the callback now that returns data (Parley will handle the conversion to UIImage etc.)
  • The ParleyNetworkSession callback won't have an Error as error completion, but a HTTPErrorResponse instead which contains the error. Since we also need the other data in some cases.
  • We'll prefix HTTPRequestMethod HTTPDataResponse HTTPErrorResponse RequestCancelable with Parley due to the possibility of name clashing in Swift.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Fixed and will be published in the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants