Skip to content

Conversation

@mschuetz-viz
Copy link
Contributor

🔗 Issue Links

Customer Support Ticket #39734

🎯 Goal

We, at Viz.ai, are heavily using URLProtocol based mocks for testing our application flows in our UI tests. While transitioning to Stream Chat we found that currently there's no way of injecting a prepared URLSessionConfiguration into the ChatClient to be used with our mocking engine. This pull request contains a proposal of how we could optionally inject such a prepared URLSessionConfiguration and unblock our development of UI tests.

📝 Summary

  • Added a non-mutable urlSessionConfiguration property to the ChatClientConfig struct
  • By default, urlSessionConfiguration will be initialised with URLSessionConfiguration.default, as before
  • In ChatClient, ChatClientConfig.urlSessionConfiguration will be used instead of URLSessionConfiguration.default
  • Added unit test to make sure that new configuration is used

🛠 Implementation

The goal was to change as little code as possible while making it obvious from an API perspective.

🎨 Showcase

not applicable - no visual changes

🧪 Manual Testing Notes

not applicable - default behaviour has not changed

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change follows zero ⚠️ policy (required)
  • This change should be manually QAed
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

🎁 Meme

giphy-downsized

@mschuetz-viz mschuetz-viz requested a review from a team as a code owner August 29, 2023 13:42
@mschuetz-viz mschuetz-viz changed the title Chore - possibility to inject URLSession to be used by ChatClient Chore - possibility to inject URLSessionConfiguration to be used by ChatClient Aug 29, 2023
Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

Thank you @mschuetz-viz!

We were actually going to work on it this week, but you beat us to it 😛

The implementation was actually what we were going to do 👍

Just added some minor comments. You don't need to change them if you don't have the time, we can do it ourselves no problem 👍

Thank you again for your time!

Comment on lines 190 to 197
/// The `URLSessionConfiguration` being used as default configuration for the `APIClient` and
/// `WebSocketClient`
public let urlSessionConfiguration: URLSessionConfiguration

public init(
apiKey: APIKey,
urlSessionConfiguration: URLSessionConfiguration = .default
) {
Copy link
Member

Choose a reason for hiding this comment

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

Just for consistency purposes it is better we follow the rest of the properties approach, where they are mutable with an initial value, and then you can change it whenever you create the config. 👍

Suggested change
/// The `URLSessionConfiguration` being used as default configuration for the `APIClient` and
/// `WebSocketClient`
public let urlSessionConfiguration: URLSessionConfiguration
public init(
apiKey: APIKey,
urlSessionConfiguration: URLSessionConfiguration = .default
) {
/// The `URLSessionConfiguration` being used as the default configuration for the `APIClient` and
/// `WebSocketClient`
public var urlSessionConfiguration: URLSessionConfiguration = .default
public init(
apiKey: APIKey
) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistency is king 🤴 - fixed

Comment on lines +246 to 249
let configuration = config.urlSessionConfiguration
configuration.waitsForConnectivity = false
configuration.httpAdditionalHeaders = sessionHeaders
configuration.timeoutIntervalForRequest = config.timeoutIntervalForRequest
Copy link
Member

Choose a reason for hiding this comment

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

One downside here is that customers won't be able to override the waitsForConnectivity or the httpAdditionalHeaders. But for now, I think this approach is simple enough, and it should work for 99.99% of use cases. If we see the need, we can add this feature later 👍

…schuetz-viz/stream-chat-swift into feature/inject-urlsessionconfiguration
@mschuetz-viz
Copy link
Contributor Author

Thanks for the quick response & input @nuno-vieira - impressive work that you're doing with this SDK, quite enjoyed looking around in your code base and try figuring out a proposal for our problem.

Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

LGTM! ✅

@mschuetz-viz Thank you again! 🙏

We will update the changelog and merge this one once we publish the 4.36.0 release 👍

@mschuetz-viz
Copy link
Contributor Author

Just to double check - some Github Checks are failing, is there anything I can do? Please let me know if you need anything further from my side.

@nuno-vieira
Copy link
Member

Just to double check - some Github Checks are failing, is there anything I can do? Please let me know if you need anything further from my side.

@mschuetz-viz It should be fine 👍 This is all permissions related things, we will handle it from here no problem 🙂

@nuno-vieira nuno-vieira merged commit a816b76 into GetStream:develop Sep 1, 2023
@mschuetz-viz
Copy link
Contributor Author

@nuno-vieira is there any estimation when this will be released? no hurry, just to plan our next sprint 😅

@nuno-vieira
Copy link
Member

nuno-vieira commented Sep 7, 2023

Hi @mschuetz-viz!

The estimation is next week, worst case scenario 2 weeks max 👍

@ipavlidakis ipavlidakis mentioned this pull request Sep 18, 2023
19 tasks
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.

2 participants