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

[expo-notifications] If unmarshaling notification request fails always try to naively reconstruct the request #10801

Merged

Conversation

sjchmiela
Copy link
Contributor

@sjchmiela sjchmiela commented Oct 26, 2020

Why

Current code doesn't try to reconstruct the notification request if we aren't able to read notification request from the existing byte array which is supposed to hold marshaled notification request. By removing the if-else in favor of …return… we ensure that the method returns a nonnull value for more arguments.

This also lets us override this method in a more natural way.

How

Instead of trying to reconstruct NotificationRequest from StatusBarNotification only if there is no ExpoNotificationBuilder.EXTRAS_MARSHALLED_NOTIFICATION_REQUEST_KEY extra value, let's try to do it if we fail to read the extras value too.

Test Plan

test-suite passes related tests.

@sjchmiela sjchmiela marked this pull request as ready for review October 26, 2020 10:09
@sjchmiela sjchmiela requested a review from cruzach as a code owner October 26, 2020 10:09
@sjchmiela sjchmiela requested a review from lukmccall October 26, 2020 10:09
Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

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

LGTM 💪💪💪

@sjchmiela sjchmiela merged commit 45728ab into master Oct 26, 2020
@sjchmiela sjchmiela deleted the @sjchmiela/expo-notifications-do-not-bail-on-invalid-requests branch October 26, 2020 10:13
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.

2 participants