-
Notifications
You must be signed in to change notification settings - Fork 2
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
Remove sharedPreferences and NSUserDefaults for storing the app id #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With "some issues" you refer to unwanted appId 'caching', correct? I'm curious why this mechanism exists, to be honest.
Yes indeed. Because of the usage of these storages, the receiver id is persisted once connected. This never seems to be overridden, and it creates an issue when the receiver ID is changed. This mechanism is in place because the app needs the receiver ID on startup. But you shouldn't manually edit the iOS/Android projects in Cordova apps. For Capacitor apps, this isn't the case anymore. So now we can configure the receiver IDs in the native configuration instead of using the workaround previously implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question remains, but still approved
Hi, We are using your fork of this plugin and implemented it recently. We had to replace https://github.com/ConnectSDK/Connect-SDK-Cordova-Plugin with a Capacitor compatible alternative. This plugin is, thanks! The proposed changes seem counter intuitive to me. The plugin gives the possibility to start a session given a certain appId:
With these changes the argument can be given but is silently ignored. I think the user should be warned that this is not the way to use this method or it should work as expected. As mentioned the plugin advocates that it implements the cast sender web SDK API. In my opion this breaks it Question: What is the use case for changing the appId? Is this used in production apps? |
Hi @wouterbin, Thank you very much for your feedback! I'm happily surprised that this fork is being used 😄 You're right. This change will make it impossible to change the appId from the web app. But there are a few reasons for this.
I will add a warning and update the readme. |
Thanks for the changes ✅ !
Yeah, it was at the time (probably still is) the most maintained fork. So it was an obvious choice 😉. Keep up the good work 👍 . Would you be interested if we stumble upon issues with the plugin (The issues on this repo are disabled now)? |
@wouterbin yes, sure! I've enabled issues. Please let us know if you have any issues. |
This PR updates the app ID configuration because we found some issues due to using NSUserDefaults and SharedPreferences.
Tested on iOS and Android devices