Add BTAppContextSwitcher.setReturnURLScheme func missing from v5 conversion

[scannillo]

Summary

We got an inbound about this function missing when using our V6 SDK from a Swift app

Changes

• Add setReturnURLScheme func, like we had in V5
• Cleanup tests to use this setter over accessing sharedInstance directly

Discussion

What used to be static in V5 (which is all of the public methods on BTAppContextSwitcher) are class methods in V6. So currently in V6, merchants need to call these functions like this: BTAppContextSwitcher.sharedInstance().handleOpenURL(context: UIOpenURLContext) or BTAppContextSwitcher.sharedInstance.handleOpenURL(URL).

I am guessing this was not intentional, since it isn't capture in our migration guide?

IMO merchants should not have access to sharedInstance and all methods on BTAppContextSwitcher should be static.

If folks agree, we'll have to decide how we want to proceed. A few options:

• Treat this as a "bug fix" and hide sharedInstance & convert methods back to static (most generous)
• Deprecate class methods & sharedInstance property + add new static methods that merchants should use

Checklist

• Added a changelog entry

Authors

@scannillo

[jaxdesmarais]

I am guessing this was not intentional, since it isn't capture in our migration guide?

Based on the original PR description (#833) the change was intentional though absolutely missed in the migration guide. From the PR:

Static wrapper methods have been removed because, in our opinion, they hid that BTAppContextSwitcher is a singleton. All invocations have been changed to directly call sharedInstance instead of the equivalent method.

Since the static methods all just wrap the singleton, I think the idea was to expose the singleton directly and avoid the use of static methods since they are only used to obfuscate that we are using a singleton. In Obj-C we had the singleton exposed publicly, the setReturnURLScheme that wraps the singleton, and a separate public returnURLScheme that could also technically be used by merchants.

I am not tied to one way or the other, just attempting to explain my memory of the reasoning from a year ago. 😅 If you feel strongly about not using the singleton directly and instead wrapping it I don't feel super strong about either approach as they are doing the same thing under the hood. Since the singleton and returnURLScheme were also public in Obj-C if we want to go with the wrapping + static approach we can likely just create the new methods and add a TODO to remove the other path in v7 to avoid a breaking change.

[scannillo]

👋 @jaxdesmarais thanks for the background here!

I get where you're coming from in wanting it to be transparent. IMO - I don't see what value the merchant gets from knowing it's a singleton. It feels weird to expose sharedInstance at all since there is nothing a merchant should be doing with it.

Also, I don't think we even need a singleton in this file. This could just be a "utility" class with all static methods & properties (see Stack Overflow).

That's just my two cents though! So going forward we can:

1. Leave everything as-is
2. Add back the static helper methods + "deprecate" the sharedInstance class methods
3. Leave a note for V7 to remove the singleton & add back static methods then

Whatever we decide we should just be sure to update the migration guide & docs. Thoughts?

[jaxdesmarais]

@scannillo -

I am fine with any of those options! I don't feel super strongly in favor/opposed to any of them, though the 1st and 3rd would be the least breaking. But even option 2 we can set up as a non breaking warning. Are you leaning one way vs the other?

[scannillo]

I am fine with any of those options! I don't feel super strongly in favor/opposed to any of them, though the 1st and 3rd would be the least breaking. But even option 2 we can set up as a non breaking warning. Are you leaning one way vs the other?

Hm - I lean toward option 2 + a note to remove the singleton in V7. It just feels a little cleaner to me. I'll make those changes to this PR and we can see how it feels.

[scannillo]

Closing due to low priority