[Push notifications] Fix for IOS 8 #2332

DannyvanderJagt · 2015-08-14T09:17:48Z

Problem
Using push notifications in IOS 8 will throw this error:
registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later.
The problem is that the check is running on compile instead of runtime.

Solution
If have changed the compile if statement to a runtime if statement. The fix is tested on: IOS 7.1 and 8.* and everything is working now.
This solution is also discussed in: #1613 and it was part of #1979. (is being separated to keep things moving)

Please let me know what you think.

ghost · 2015-08-15T20:32:46Z

❤️

seidtgeist · 2015-08-17T09:24:13Z

Thank you @DannyvanderJagt, this worked for me. Nitpicking: Indentation needs to be adjusted now that it's a statement?

DannyvanderJagt · 2015-08-17T10:43:03Z

@ehd Glad it worked and you're right, I have fixed it.

seidtgeist · 2015-08-17T13:53:30Z

Thanks again :)

bleonard · 2015-08-29T22:39:46Z

This is the fix that i was going to suggest for the issue i'm seeing as well. The issue is that the npm_module seems to define the deploy target itself as 7.0 so the #if will never work - a runtime check like in this PR is necessary.

anthonywebb · 2015-09-16T17:28:23Z

Man, really need this fix, no merge yet?

DannyvanderJagt · 2015-09-17T07:09:30Z

@anthonywebb No merge yet, but this issue is easy to fix if you don't want to wait. I have also written a tutorial about How to use push notifications in React-Native for IOS, it also includes an explanation on how to fix this bug yourself.

@nicklockwood @sahrens @brentvatne I know that you are very busy at the moment. However without this fix push notifications aren't working for IOS in React-Native. This fix is also discussed multiple times in the last few months (#1613, #1979, etc) and therefore it should be easy to review and merge this. Let me know if I can help you guys somehow.

jacob-israel-turner · 2015-09-22T12:56:53Z

+1, could have used this fix. :)

kexhest · 2015-09-28T19:58:57Z

+1

ericvicenti · 2015-09-28T22:51:55Z

Libraries/PushNotificationIOS/RCTPushNotificationManager.m

+    id notificationSettings = [UIUserNotificationSettings settingsForTypes:types categories:nil];
+    [[UIApplication sharedApplication] registerUserNotificationSettings:notificationSettings];
+    [[UIApplication sharedApplication] registerForRemoteNotifications];
+  }else{


nit: spaces around else

DannyvanderJagt · 2015-09-29T07:29:57Z

@ericvicenti I have added the spaces. However when I did a rebase git added the 'Merge remote-tracking' commit. Let me know if that's a problem.

ide · 2015-09-29T07:42:06Z

Can you clean up your branch? Seems like something went wrong when you rebased. Also please squash your commits when you're ready for review.

DannyvanderJagt · 2015-09-29T09:34:48Z

@ide I have cleaned up the branch and squashed the commits. I also have updated the code of this PR, it is now using app instead of [UIApplication sharedApplication].

ericvicenti · 2015-09-29T21:22:52Z

@facebook-github-bot shipit

jaygarcia · 2015-09-29T23:10:36Z

XD. Facebook github bot is stuck on stupid ;)

jibonpabna · 2015-09-29T23:21:15Z

Libraries/PushNotificationIOS/RCTPushNotificationManager.m

-  [app registerUserNotificationSettings:notificationSettings];
-  [app registerForRemoteNotifications];
-#else
-  [app registerForRemoteNotificationTypes:types];


facebook-github-bot · 2015-09-29T23:23:32Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1056653971043101/int_phab to review.

vjeux · 2015-09-29T23:26:46Z

Whoops, sorry for the spam :x We need to fix that edge case :)

ericvicenti · 2015-09-30T00:10:33Z

This failed our internal tests due to the following:

@frantic, any thoughts on why the OSS tests pass but the internal ones do not?

DannyvanderJagt · 2015-09-30T09:55:34Z

@ericvicenti The OSS is giving a warning instead of an error and therefore it passes.

I have fixed the actual problem/error. The problem is actually that types need to be converted to enum UIUserNotificationType. To accomplish this we need to replace types with (NSUInteger)types.

The result will be:

  UIApplication *app = RCTSharedApplication();
  if ([app respondsToSelector:@selector(registerUserNotificationSettings:)]) {
    id notificationSettings = [UIUserNotificationSettings settingsForTypes:(NSUInteger)types categories:nil];
    [app registerUserNotificationSettings:notificationSettings];
    [app registerForRemoteNotifications];
  } else {
    [app registerForRemoteNotificationTypes:(NSUInteger)types];
  }

I've tested it and it works just as it should.
I would like to hear to hear what you think before I add this.

ericvicenti · 2015-09-30T15:50:58Z

Looks great!

Small note: use the UIUserNotificationSettings type instead of id

DannyvanderJagt · 2015-09-30T16:25:14Z

@ericvicenti Thanks for the note.
I have replaced id notificationSettings with UIUserNotificationSettings * notificationSettings.

I have squashed and rebased the PR.
Let me know if there is anything else I can do.

@ericvicenti

Summary: **Problem** Using push notifications in IOS 8 will throw this error: `registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later.` The problem is that the check is running on compile instead of runtime. **Solution** If have changed the compile if statement to a runtime if statement. The fix is tested on: IOS 7.1 and 8.* and everything is working now. This solution is also discussed in: facebook#1613 and it was part of facebook#1979. (is being separated to keep things moving) Please let me know what you think.Closes facebook#2332 Reviewed By: @svcscm Differential Revision: D2490987 Pulled By: @ericvicenti

@ericvicenti

Summary: **Problem** Using push notifications in IOS 8 will throw this error: `registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later.` The problem is that the check is running on compile instead of runtime. **Solution** If have changed the compile if statement to a runtime if statement. The fix is tested on: IOS 7.1 and 8.* and everything is working now. This solution is also discussed in: facebook#1613 and it was part of facebook#1979. (is being separated to keep things moving) Please let me know what you think.Closes facebook#2332 Reviewed By: @svcscm Differential Revision: D2490987 Pulled By: @ericvicenti

@ericvicenti

Summary: **Problem** Using push notifications in IOS 8 will throw this error: `registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later.` The problem is that the check is running on compile instead of runtime. **Solution** If have changed the compile if statement to a runtime if statement. The fix is tested on: IOS 7.1 and 8.* and everything is working now. This solution is also discussed in: facebook#1613 and it was part of facebook#1979. (is being separated to keep things moving) Please let me know what you think.Closes facebook#2332 Reviewed By: @svcscm Differential Revision: D2490987 Pulled By: @ericvicenti

@ericvicenti

Summary: **Problem** Using push notifications in IOS 8 will throw this error: `registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later.` The problem is that the check is running on compile instead of runtime. **Solution** If have changed the compile if statement to a runtime if statement. The fix is tested on: IOS 7.1 and 8.* and everything is working now. This solution is also discussed in: facebook#1613 and it was part of facebook#1979. (is being separated to keep things moving) Please let me know what you think.Closes facebook#2332 Reviewed By: @svcscm Differential Revision: D2490987 Pulled By: @ericvicenti

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 14, 2015

DannyvanderJagt mentioned this pull request Aug 14, 2015

[PushNotificationIOS] No register event ever gets triggered #1613

Closed

DannyvanderJagt mentioned this pull request Aug 18, 2015

Pushnotifications events and fix for AppDelegate #1979

Closed

DannyvanderJagt mentioned this pull request Sep 17, 2015

[Pushnotifications] Add error event #2336

Closed

ericvicenti reviewed Sep 28, 2015
View reviewed changes

ericvicenti self-assigned this Sep 28, 2015

jibonpabna reviewed Sep 29, 2015
View reviewed changes

[Push notifications] Fix register for IOS push notifications

2bd8885

ghost closed this in 200b811 Oct 1, 2015

bleonard mentioned this pull request Oct 8, 2015

Registering Push Notifications Twice #3288

Closed

nb7123 mentioned this pull request Oct 12, 2015

registerForRemoteNotificationTypes is not support in IOS 8.0 and later #3351

Closed

This pull request was closed.

[Push notifications] Fix for IOS 8 #2332

[Push notifications] Fix for IOS 8 #2332

Uh oh!

Conversation

DannyvanderJagt commented Aug 14, 2015

Uh oh!

ghost commented Aug 15, 2015

Uh oh!

seidtgeist commented Aug 17, 2015

Uh oh!

DannyvanderJagt commented Aug 17, 2015

Uh oh!

seidtgeist commented Aug 17, 2015

Uh oh!

bleonard commented Aug 29, 2015

Uh oh!

anthonywebb commented Sep 16, 2015

Uh oh!

DannyvanderJagt commented Sep 17, 2015

Uh oh!

jacob-israel-turner commented Sep 22, 2015

Uh oh!

kexhest commented Sep 28, 2015

Uh oh!

ericvicenti Sep 28, 2015

Choose a reason for hiding this comment

Uh oh!

DannyvanderJagt commented Sep 29, 2015

Uh oh!

ide commented Sep 29, 2015

Uh oh!

DannyvanderJagt commented Sep 29, 2015

Uh oh!

ericvicenti commented Sep 29, 2015

Uh oh!

jaygarcia commented Sep 29, 2015

Uh oh!

jibonpabna Sep 29, 2015

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 29, 2015

Uh oh!

vjeux commented Sep 29, 2015

Uh oh!

ericvicenti commented Sep 30, 2015

Uh oh!

DannyvanderJagt commented Sep 30, 2015

Uh oh!

ericvicenti commented Sep 30, 2015

Uh oh!

DannyvanderJagt commented Sep 30, 2015

Uh oh!

Uh oh!