react-native-push-notification / ios

React Native Push Notification API for iOS.
MIT License
740 stars 284 forks source link

Add ability to repeat notifications like on iOS #268

Closed ghivert closed 3 years ago

ghivert commented 3 years ago

Hi,

Today the repeats option does not work. This PR fix this, and add the ability to freely choose the repeating interval, like asking in #263. The idea is rather simple: let the user choose how they want to repeat the notifications with a repeatsComponent field in the request, and converting the components to NSCalendarUnit to have the exact same behavior in JS than what we have in Objective-C. In case there’s no repeats, then the old behavior is used, with the same NSCalendarUnit to ensure the notification will be sent at the correct date.

Tell me if it suits you. If not, I would be happy to fix what you want. :)

P0SEID0N commented 3 years ago

Any update on getting this into the build? I am working on a project where we need access to weekly repeat intervals. I would prefer to be able to schedule notifications locally rather than needing to rely on internet connection to compensate with a server backend.

Thanks!

ghivert commented 3 years ago

I personnaly ended up with my personal repo, for my own needs while the maintainers are not available. The package is perfectly working on my app fyi, with the notifications displayed at the correct time and hour. The code modification is really small, so you can easily drop it when the bug will be fixed in the future.

iamharky commented 3 years ago

Thank you so much @ghivert ! Your solution seems to work for me, perfectly repeats notifications.

batuhansahan commented 3 years ago

Any update on getting this into the build? @Dallas62 @Naturalclar

Dallas62 commented 3 years ago

Hi, I'm not the maintainer of this repository. But I do not understand why we should be available to repeat the notification from nanoseconds ? And why can we repeat the notification by month and hour ? If it's hourly, by definition it will go through months and days. A unique type of repeating should be enough. Regards

smadan commented 3 years ago

Pinging this again - I want to use this for my app. Can one of the maintainers approve this change? @Naturalclar?

ghivert commented 3 years ago

Hi, I'm not the maintainer of this repository. But I do not understand why we should be available to repeat the notification from nanoseconds ? And why can we repeat the notification by month and hour ? If it's hourly, by definition it will go through months and days. A unique type of repeating should be enough. Regards

@Dallas62 Hi, No problem with your questions, I had the same interrogations when writing the PR. The best you can do is playing with the iOS API. It's not really clear at first sight why it's doing, but it allows you to be really precise for your notifications. You can for example specify the hours for specific months. That's why you can use months and hours.

My idea was to mimic the behavior provided by iOS in this package, because we're only dealing with iOS and it would be too bad to ignore some features because of some reasons. You can easily write a wrapper talking both to iOS and Android with this package as foundation. Of course I'm open to debate, but I think keeping this feature as a wrapper is better than providing a custom solution: I really don't know the different use cases for notifications and it's probably easy to forget about some users.


Pinging this again - I want to use this for my app. Can one of the maintainers approve this change? @Naturalclar?

@smadan The package hadn't changed since I wrote the PR, so you can probably use ghivert/push-notification-ios#2b9a66b in your package.json while the PR is pending, and switch back to the package when the merge is done. Your only risk is to have some API changes when the PR will be merged, but if you need this for work, this can be worth it.

P0SEID0N commented 3 years ago

Hi, I'm not the maintainer of this repository. But I do not understand why we should be available to repeat the notification from nanoseconds ? And why can we repeat the notification by month and hour ? If it's hourly, by definition it will go through months and days. A unique type of repeating should be enough. Regards

I agree that nanoseconds are not really necessary as there really shouldnt be any situations where you're emitting notification events in nanosecond intervals.

On your comment about month and hour. I agree somewhat, but if you're thinking logistically about it, it is possible to have a notification go off at specific hours but not recast that notification by the month. For example if you cast by Month and Hour there might be situations where your notifications do not emit at the expected time. However all of these things are edge cases that can easily be avoided with some very lightweight code. However I see what the original PR creator was trying to do, I have done some digging myself into IOS notifications and they did mimic how IOS's OS handles notifications which I can respect (keeping the fundamentals the same that is).

I do not disagree that we can optimize the code here though. @Dallas62 Do you plan on use this PR for your notification library? I was having an issue with repeating notifications and had planned to manually change your own Package to use this fork (or a similar edit). Repeating intervals is somewhat important for timers and such.

jamesxabregas commented 3 years ago

So I wish I has come across this PR before I spent the time on implementing much the same thing... I just submitted a PR recently #308. Your approach seems to be a bit more configurable, I just sought to replicate the repeatInterval functionality that is available in scheduleLocalNotification(). It would be good if the project maintainer could take a look at these PRs. At the moment the notification does not repeat at all despite what the documentation says.

jamesxabregas commented 3 years ago

You should probably also add in NSCalendarUnitWeekday as one of the NSDateComponents as scheduling reoccuring weekly notifications is a fairly common requirement.

Naturalclar commented 3 years ago

@ghivert @jamesxabregas Hello, I'm the maintainer of this library, I'm so sorry for not being able to respond earlier.

This would be a great addition, and would love to merge this in. @ghivert, could you apply the suggestions by @jamesxabregas ? Once that's done I'll do some testing and publish the library

Overall I think this is good. My suggestions would be to also allow repeating by year and dayOfWeek, but to remove nanosecond. Even if nanosecond repeats are supported in iOS, I'm not sure if this library should support it.

I agree that nanosecond should be removed as well

ghivert commented 3 years ago

Hi, I'll do the change, but I can't do them right now. I can find some time during to next one or two weeks.

Naturalclar commented 3 years ago

No rush 😁 Thanks so much!

batuhansahan commented 3 years ago

@ghivert Thanks so much!!!