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

Migration to v5 queries #745

Open
Enricoza opened this issue Dec 17, 2024 · 5 comments
Open

Migration to v5 queries #745

Enricoza opened this issue Dec 17, 2024 · 5 comments

Comments

@Enricoza
Copy link

I was migrating from v4 to v5 and had a couple quick questions for the team.

A general question regarding constants

  1. Why did you remove all of the constants? They were quite useful to avoid copy paste mistakes.

Regarding the URL Strategy

I'm unsure if I understood the changes correctly.

  1. Is the change from a strategy to an array of domains just due to the fact that some of the strategies have multiple domains (e.g. ADJUrlStrategyChina, ADJUrlStrategyIndia, ADJUrlStrategyCn) or could I pass now an array like this: ["eu.adjust.com", "tr.adjust.com"]?
  2. Are the other two parameters useSubdomains and isDataResidency fixed based on the array of domains passed in the first parameter? Or could I pass for example adjustConfig.setUrlStrategy(["eu.adjust.com"], useSubdomains: false, isDataResidency: false) or equivalently adjustConfig.setUrlStrategy(["adjust.net.in", "adjust.com"], useSubdomains: false, isDataResidency: true)?
  3. If they are fixed, why isn't that part of the AdjustSDK to infer or generate? Something like the following, for example, would avoid any possible confusion here:
public struct AdjustUrlStrategy {
    let domains: [String]
    let useSubdomains: Bool
    let isDataResidency: Bool
    public init(...) { ... }
}

public extension AdjustUrlStrategy {
    static let euResidency = AdjustUrlStrategy(domains: ["eu.adjust.com"], useSubdomains: true, isDataResidency: true)
    static let trResidency = AdjustUrlStrategy(domains: ["tr.adjust.com"], useSubdomains: true, isDataResidency: true)
    ...
    static let indiaStrategy = AdjustUrlStrategy(domains: ["adjust.net.in", "adjust.com"], useSubdomains: true, isDataResidency: false)
}

public class AdjustConfig {
    public func setUrlStrategy(_ strategy: AdjustUrlStrategy) { ... }
}

If this were to be implemented this way instead of the 5.0 version, it would give the same amount of parametrization in case some custom domain needs to be added but without the need for your customers to manually insert the ones that are already there, for example:

// standard strategy
adjustConfig.setUrlStrategy(.euResidency)
// or for custom strategy:
adjustConfig.setUrlStrategy(AdjustUrlStrategy(domains: ["my.custom.domain.com"], useSubdomains: false, isDataResidency: true))

Of course this only makes sense if the assumptions i made in point 1 to 3 are correct and there are some standard strategies.

TrackAdRevenue

For the trackAdRevenue a Data payload parameter was removed (as i can see from the code) but I can't find it in the migration guide. Should the payload be just removed or should it be passed in some other way?

Thank you very much for the support and for the migration guide which was very helpful so far.

@uerceg
Copy link
Contributor

uerceg commented Dec 17, 2024

Hey @Enricoza,

Thanks for the reaching out.

Why did you remove all of the constants? They were quite useful to avoid copy paste mistakes.

Fair observation. However, even though we were aware that the chances of typos and copy / paste actions are going up, we thought that it's a fair tradeoff for having the flexibility not to depend on certain SDK version for current (and future) URL strategies and data residencies which we will be enabling on our backend. One other consideration was that on our end we also need to ship 12 SDK updates just for sake of propagating that to all the SDKs we have.

Is the change from a strategy to an array of domains just due to the fact that some of the strategies have multiple domains (e.g. ADJUrlStrategyChina, ADJUrlStrategyIndia, ADJUrlStrategyCn) or could I pass now an array like this: ["eu.adjust.com", "tr.adjust.com"]?

Change we made is pretty much just translating v4 constants we had into a certain combination of these three parameters. If you check the table in this chapter, it's depicting exactly how the combination of these parameters should look like, in case you were using some of these v4 constants. Depending on the constant you were using, you should invoke setUrlStrategy method exactly like that table suggests to preserve behavior you had with v4.

question 2 and 3

Like mentioned above, the boolean parameters need to be set exactly in accordance to the content of the array. Yes, in theory SDK could understand that if we see ["eu.adjust.com"] array that this isDataResidency should be true. But then we are back to square one and we would not be able to automatically set this in our SDK once we add some new data residency URL domain xx.adjust.com without actually shipping an SDK update for that. And with that suggestion, there would be a possibility to use the same data residency URL strategy in two different ways:

// #1
adjustConfig.setUrlStrategy(.xxResidency)
// #2
adjustConfig.setUrlStrategy(AdjustUrlStrategy(domains: ["xx.adjust.com"], useSubdomains: true, isDataResidency: true))

which is not something we really prefer (it makes no harm, of course, but just for sake of consistency).

I do agree that abstracting all these three variables behind some kind of ADJUrlStrategy class would be more elegant way of doing things, but it would make things nicer style wise, fundamentally - not really.

But also to stress one more time -> all the URL strategies and data residency URL strategies which we support are to be used exactly like stated in the table from the documentation.

For the trackAdRevenue a Data payload parameter was removed (as i can see from the code) but I can't find it in the migration guide. Should the payload be just removed or should it be passed in some other way?

Are you referring to this method?

@Enricoza
Copy link
Author

Interesting points regarding the constants and your preferences about them. But ok, thanks for clarifying.

Anyway yes, that's the method I was referring to. Could you clarify on that too?

Thank you again @uerceg for the quick response!

@uerceg
Copy link
Contributor

uerceg commented Dec 18, 2024

@Enricoza I am curious what are you using that method for?

That method was added as a first method for tracking ad revenue with MoPub (that was our first partner we integrated with on this front). But after that MoPub use case, we noticed that with adding more partners, that API was not flexible enough (since we kinda made it MoPub specific), so we went with more generic one which pretty much exists in same form in v5 as well (minus the dropping the constants change).

MoPub is no longer around, but instead those users have migrated to AppLovin MAX solution, so makes me wonder what was your use case for that method nowadays.

@Enricoza
Copy link
Author

@uerceg I couldn't tell you what I'm using that for as I'm developing an integration, not the actual usage of the library. The old method was still supported (as it was in v4 here) for our customers to use but probably not used that much according to what you are saying. Anyway I understand now. Thanks.

@uerceg
Copy link
Contributor

uerceg commented Dec 18, 2024

I see, thanks for the comments. But yeah, I agree that we should probably add the clarification comment about that method in migration guide as well. It might have been skipped due to the reason mentioned above -> even within SDK v4, trackAdRevenue:payload: should have been deprecated and abandoned in favor of trackAdRevenue:. So if you were in doubt whether that method should be exposed to the outer world in some other way - nope, feel free to skip it.

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

No branches or pull requests

2 participants