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

feat(ios): Replace usage of UserDefaults with KeyValueStore. #7191

Merged
merged 3 commits into from
Jan 17, 2024
Merged

Conversation

Steven0351
Copy link
Member

This will prevent every single Capacitor user from having to declare use of a required reason API when it becomes required by Apple. This is also made available for plugin authors or others who utilize Capacitor as a dependency.

… will prevent every single

Capacitor user from having to declare use of a required reason API. This
is also made available for plugin authors or others who utilize
Capacitor as a dependency.
try? FileManager.default.createDirectory(at: url, withIntermediateDirectories: true, attributes: nil)

let new = FileStore(baseUrl: url)
instances[url.absoluteString] = new
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly not sure why I'm keying off the URL instead of just the name. That would allow for checking for its existence before doing all the other url logic.

Copy link
Member

@markemer markemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already talked about this a bunch, but it also works when I run it, so that's good.

@jcesarmobile
Copy link
Member

This will break live updates for users using cordova-plugin-ionic, because the plugin is storing in UserDefaults the values being read by Capacitor, so if the plugin still uses UserDefaults but Capacitor don't read those values then the plugin will not work.
Do we plan to force users to use the new Capacitor live updates plugin with Capacitor 6 instead of cordova-plugin-ionic?
If not, this is not something we can merge.

@Steven0351
Copy link
Member Author

Steven0351 commented Jan 16, 2024

This will break live updates for users using cordova-plugin-ionic, because the plugin is storing in UserDefaults the values being read by Capacitor, so if the plugin still uses UserDefaults but Capacitor don't read those values then the plugin will not work. Do we plan to force users to use the new Capacitor live updates plugin with Capacitor 6 instead of cordova-plugin-ionic? If not, this is not something we can merge.

cordova-plugin-ionic is not actually reading or setting any values being set by Capacitor core through UserDefaults in this case. For the serverBasePath stuff it actually goes through Ionic.WebView https://github.com/search?q=repo%3Aionic-team%2Fcordova-plugin-ionic+serverBasePath&type=code which should be routing to the Capacitor WebView plugin. All other use of UserDefaults in cordova-plugin-ionic is for its own data, which has no impact on core.

@jcesarmobile
Copy link
Member

Ah, sorry, I was thinking about lastBinaryVersionCode and lastBinaryVersionName, not serverBasePath, but looks like they are not stored by cordova-plugin-ionic neither, it stores binaryVersionName and binaryVersionCode, without last prefix, so they are different things and we should be safe.

@Steven0351
Copy link
Member Author

Ah, sorry, I was thinking about lastBinaryVersionCode and lastBinaryVersionName, not serverBasePath, but looks like they are not stored by cordova-plugin-ionic neither, it stores binaryVersionName and binaryVersionCode, without last prefix, so they are different things and we should be safe.

No worries, I immediately thought of cordova-plugin-ionic when I started this work and tried to be very thorough in my vetting of its uses.

@markemer markemer merged commit cd58ba2 into main Jan 17, 2024
6 checks passed
@markemer markemer deleted the kvstore branch January 17, 2024 15:56
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

Successfully merging this pull request may close these issues.

4 participants