feat: Proxy server header capability #628
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tance creation for ProxyServerConfig & ServerURL
zihejia
reviewed
Mar 29, 2024
| optOutTrackingByDefault: Bool = false, | ||
| useUniqueDistinctId: Bool = false, | ||
| superProperties: Properties? = nil, | ||
| proxyServerConfig: ProxyServerConfig) -> MixpanelInstance { |
There was a problem hiding this comment.
if proxyServerConfig is optional, could you add proxyServerConfig: ProxyServerConfig? = nil to the existing initialize function rather than adding a new function?
There was a problem hiding this comment.
Thanks for the feedback on the PR 🙌 . On using the same intialize function, they are few points I want to highlight.
- By combining
serverURLandProxyServerConfiginto one method, we risk users setting both parameters, which doesn't align with their intended use. Ideally one of them should be set. Keeping them separate ensures users have a clear choice without the possibility of configuring conflicting options. - With two distinct initialization methods, we're directly guiding users on their options. One method is for basic server URL configuration, and the other is for more advanced proxy configurations.
|
thank you so much @mohitanand-cred ! the PR looks good except one comment, also could you rebase/merge from the latest master and make sure all CI tests pass? |
…nto feat/proxy_server_header_capability
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes:
headerandquery itemsparameter.ProxyServerConfigwhich takes in proxy server url and a delegate(MixpanelProxyServerDelegate).MixpanelProxyServerDelegatecan be used to provide headers or query items to the proxy server. The delegate funcmixpanelResourceForProxyServer(_ mixpanel: MixpanelInstance) -> ServerProxyResource?is being called when its time flush the batch request.initializefunction in Mixpanel which takes inProxyServerConfiginstead of justserverURL.