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.

Sign up for GitHub

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

Rename POST_NOTIFICATION to POST_NOTIFICATIONS #35533

Closed
wants to merge 1 commit into from
Closed

Rename POST_NOTIFICATION to POST_NOTIFICATIONS #35533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions Libraries/PermissionsAndroid/PermissionsAndroid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ const PERMISSIONS = Object.freeze({
ANSWER_PHONE_CALLS: 'android.permission.ANSWER_PHONE_CALLS',
READ_PHONE_NUMBERS: 'android.permission.READ_PHONE_NUMBERS',
UWB_RANGING: 'android.permission.UWB_RANGING',
POST_NOTIFICATION: 'android.permission.POST_NOTIFICATIONS',
POST_NOTIFICATION: 'android.permission.POST_NOTIFICATIONS', // Remove in 0.72
POST_NOTIFICATIONS: 'android.permission.POST_NOTIFICATIONS',
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for spotting this. I'm in for a breaking change. The problem is that we shipped this in 0.70 as well, so we'll need to backport this.

Potentially having both is a preferred approach and in 0.72 we can remove the duplicate? (cc @kelset for visiblity)

Copy link
Contributor

Choose a reason for hiding this comment

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

making sure that I'm following: you want to merge this in main and then cherry pick for both 0.71 and 0.70? I'm ok with that, let's make sure to post comments with links in both release discussions

Copy link
Contributor

Choose a reason for hiding this comment

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

Asking for your opinion as this is essentially a 'typo' we let pass through. We can add both POST_NOTIFICATION and POST_NOTIFICATIONS in the JS API and have the breaking change in .72 OR have this as a breaking change in both .70 and .71

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not do breaking changes in 0.70 since it's already out. For 0.71 we are still in time to add as a BC. For 0.72 is no problem.

Copy link
Contributor

@kelset kelset Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

that said, my personal preference would be no breaking changes in 0.70 and 0.71, but yes for 0.72 - but I'm not sure what that would imply here

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so let's merge this and pick it on 0.70/0.71
Then let's do the breaking change on main for 72

NEARBY_WIFI_DEVICES: 'android.permission.NEARBY_WIFI_DEVICES',
});

Expand Down Expand Up @@ -106,7 +107,8 @@ class PermissionsAndroid {
CAMERA: string,
GET_ACCOUNTS: string,
NEARBY_WIFI_DEVICES: string,
POST_NOTIFICATION: string,
POST_NOTIFICATION: string, // Remove in 0.72
POST_NOTIFICATIONS: string,
PROCESS_OUTGOING_CALLS: string,
READ_CALENDAR: string,
READ_CALL_LOG: string,
Expand Down
8 changes: 4 additions & 4 deletions packages/rn-tester/js/examples/PermissionsAndroid/PermissionsExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,13 @@ function PermissionsExample() {
style={styles.option}
/>
<RNTOption
label={PermissionsAndroid.PERMISSIONS.POST_NOTIFICATION}
key={PermissionsAndroid.PERMISSIONS.POST_NOTIFICATION}
label={PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS}
key={PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS}
onPress={() =>
setPermission(PermissionsAndroid.PERMISSIONS.POST_NOTIFICATION)
setPermission(PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS)
}
selected={
permission === PermissionsAndroid.PERMISSIONS.POST_NOTIFICATION
permission === PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS
}
style={styles.option}
/>
Expand Down