Refactor to take params as a function instead, to allow injection of tokens (or other params) that has changed upon reconnect #153
Conversation
|
Thanks for the PR. The phoenix.js client has similar functionality so I think it's important to get this behavior in the swift client as well. Upon initial review, i don't like completely replacing the |
|
I was thinking the same, but I'm not sure how to go about it. I had to fix it quickly because of a bug, but since I have forked it I can migrate to the final version later. I'm going to try to add another convenience init to wrap the old params in a function so it works as intended in the old case too. |
|
Made some more changes, and due to how it now rebuilds the endPoint each time I edited the way it asserts in the test that '/websocket' has been appended. I hope that's okay, and if not further changes may have to be made. All tests pass though, but I guess one should add a few more. Might do that when I have another minute later today :) |
Sources/client/Socket.swift
Outdated
| public convenience init(_ endPoint: String, | ||
| params: [String: Any]? = nil) { | ||
| params: Payload?) { |
There was a problem hiding this comment.
I would prefer to see this init signature stay like params: Payload? = nil and then change paramsGetter: PayloadGetterFunction? to not have a default nil. Then we can remove the added convenience init and not introduce a change to the client's current API
There was a problem hiding this comment.
What do you do if you provide both?
Socket("https://somewhere", params: ["hello":"world"], { ["hello":"another world", "and_more": "stuff"] })
This isn't clear to me, and the API is not very clear if this is the way to go. If Socket(: String, params: [:]) instead causes a deprecation warning and we introduce Socket(: String, paramsClosure: [:]) that's a lot clearer.
That's the reason I did what I did.
There was a problem hiding this comment.
Is this a valid case? Likely a client that used a PayloadClosure will provide PayloadClosure that returns the correct initial payload and client that provides just Payload will not need to use the dynamic closure.
I wouldn't say deprecating params: Payload? init is necessary
…etterFunction to PayloadClosure
…ty could possibly not reflect the actual URL used at connection time.
…msClosure: Payload?) not supply a default parameter for paramsClosure
We had an issue where our iPads would disconnect from Phoenix and it would result in an infinite loop of retries, as the socket would reuse the old params, and you couldn't change it between attempts. To solve this I mad these changes, which unfortunately breaks the API but it could be introduced in 1.2 instead, or something.
It could also be applied to the channels, as well, if necessary.
I have not edited the tests yet, as I have not installed Carthage so I couldn't build it at the time. I will do so if we decide that this is a change we want for this library.
Thoughts?