vonovak / react-native-add-calendar-event

Create, view or edit events in react native using the standard iOS / Android dialogs
MIT License
344 stars 103 forks source link

fix: Update permission requests for iOS 17+ #181

Closed jgillick closed 8 months ago

jgillick commented 10 months ago

This PR fixes the module for iOS 17+. The previous method (requestAccessToEntityType) simply fails for iOS 17+.

The update uses the new methods to either ask for full access (edit/view actions) or write-only access (event creation).

vonovak commented 10 months ago

Hello @jgillick and thanks for the PR, to be honest, I'd rather have this lib not do any permissions-related stuff and defer that task to https://github.com/zoontek/react-native-permissions

What are your thoughts on this? Thank you 🙂

jgillick commented 10 months ago

That's a phenomenal suggestion and should simplify this quite library a bit. I'll take a look at doing that this week.

jgillick commented 10 months ago

I'm assuming you don't want to load react-native-permissions in the lib, and instead, leave permission checking to the implementers.

vonovak commented 10 months ago

Yes, exactly 🙂

jgillick commented 10 months ago

I've updated the PR to remove permissions checking. I also had to update the example app to a more recent version of React Native to work with the latest version of react-native-permission -- so I just created a fresh example app and ported the previous app code over to it. The example app uses the latest beta of react-native-permission since that one supports iOS 17+.

I did these changes across two commits, so you could see the lib changes in an isolated way since there's so much boilerplate to the react native app.

Since this version has breaking changes (permission management moving out of the lib), I bumped the version up to 5.0.

jgillick commented 10 months ago

I only expect one 'eventsdemo' project to be present. Can you please fix that? Thanks!

Ah, that's because I had named it all lowercase, and apple file systems aren't case-sensitive, so the casing change didn't get committed to git. I've changed it back to camel case, let me know if it works as expected now.

vonovak commented 9 months ago

hello @jgillick and thanks for your patience, this is what I get when I want to build the project. Not sure what went wrong here, maybe still something with the casing?

Screenshot 2023-11-26 at 21 07 42

I'd like to make a suggestion: instead of setting up the example project the way it's done now, would you be up for migrating it to https://github.com/microsoft/react-native-test-app?

You can just follow the way it's done in my other repos such as here https://github.com/vonovak/react-native-theme-control

The example folder is very simple, it has just a few files. Then there are some changes to be done in metro config https://github.com/vonovak/react-native-theme-control/blob/main/metro.config.js

and in RN config https://github.com/vonovak/react-native-theme-control/blob/main/react-native.config.js

when this is done, that will make the module maintenance much easier.

Hope this is okay, thank you! :)

rvasseur31 commented 9 months ago

If you remove the permission check in the library over react-native-permission, I think it will break expo apps. This PR https://github.com/vonovak/react-native-add-calendar-event/pull/182 works for me with expo

vonovak commented 9 months ago

expo go apps should use https://docs.expo.dev/guides/permissions/

ChronoByteCosmonaut commented 8 months ago

Any updates on this @vonovak @jgillick ?

jgillick commented 8 months ago

@vonovak Unfortunately react-native-test-app won't work because, as far as I can tell, you cannot modify the Info.plist file of the generated app. For react-native-permissions to work, this file needs to be modified. (see feature request)

I deleted the example directory entirely and recreated the example app from scratch again. Maybe it'll work for you this time.

To make it work, I have to reference the module from my fork in the package.json file ("react-native-add-calendar-event": "jgillick/react-native-add-calendar-event",). This will need to be changed back to point to the main branch module before merging.

For anyone waiting for this PR to be merged, you can install the fork directly by adding it to your dependencies like this:

{
 ...
  "dependencies": {
    ...
    "react-native-add-calendar-event": "jgillick/react-native-add-calendar-event",
    ...
  },
 ...
}
vonovak commented 8 months ago

@jgillick as the feature request you linked mentions, you can use a config plugin, but for this use case, I'll be happy with a simple patch-package patch. So the start command would run npx patch-package && npx react-native start.

The patch is applied to the default info.plist and adds the permissions descriptions as needed.

Maintaining a patch file is much easier than upgrade react native project every few months. Would you be open to making that change? Thank you

jgillick commented 8 months ago

@vonovak I've integrated react-native-test-app. At this point, I'd recommend that any other suggestions not relating to the fix for iOS 17+ get moved to another PR.

ChronoByteCosmonaut commented 8 months ago

Any updates on this?

vonovak commented 8 months ago

continued here https://github.com/vonovak/react-native-add-calendar-event/pull/183

thank you for the effort! :)