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

Rename POST_NOTIFICATION to POST_NOTIFICATIONS #35533

Closed
wants to merge 1 commit into from
Closed

Rename POST_NOTIFICATION to POST_NOTIFICATIONS #35533

wants to merge 1 commit into from

Conversation

dcangulo
Copy link
Contributor

@dcangulo dcangulo commented Dec 2, 2022

Summary

After adding <uses-permission android:name="android.permission.POST_NOTIFICATIONS"/> on my AndroidManifest.xml, I expected to use PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS but POST_NOTIFICATIONS is undefined and is named POST_NOTIFICATION instead.

Every other Android permission is 1:1 in spelling except this one where it lacks S.

Not sure if this is a welcome change since this can be breaking. Or maybe we can include both with and without S to not be a breaking change. Or just keep it as is and close this PR.

Changelog

[Android] [Changed] - Rename POST_NOTIFICATION to POST_NOTIFICATIONS

Test Plan

PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS should not be undefined.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 2, 2022
@react-native-bot react-native-bot added the Platform: Android Android applications. label Dec 2, 2022
@analysis-bot
Copy link

analysis-bot commented Dec 2, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,102,159 -35
android hermes armeabi-v7a 6,470,950 -27
android hermes x86 7,520,368 -33
android hermes x86_64 7,378,725 -34
android jsc arm64-v8a 8,969,961 +14
android jsc armeabi-v7a 7,701,357 +10
android jsc x86 9,032,670 +7
android jsc x86_64 9,510,150 +5

Base commit: 436da18
Branch: main

@analysis-bot
Copy link

analysis-bot commented Dec 2, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 436da18
Branch: main

@pull-bot
Copy link

pull-bot commented Dec 2, 2022

PR build artifact for fd3cf8a is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

pull-bot commented Dec 2, 2022

PR build artifact for fd3cf8a is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@@ -75,7 +75,7 @@ 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_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

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

@pull-bot
Copy link

pull-bot commented Dec 2, 2022

PR build artifact for 28147f8 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

pull-bot commented Dec 2, 2022

PR build artifact for 28147f8 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@dcangulo
Copy link
Contributor Author

dcangulo commented Dec 2, 2022

@cortinico Restored to not be a breaking change.

@facebook-github-bot
Copy link
Contributor

@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Dec 9, 2022
@facebook-github-bot
Copy link
Contributor

@philIip merged this pull request in 910a750.

kelset pushed a commit that referenced this pull request Dec 13, 2022
Summary:
After adding `<uses-permission android:name="android.permission.POST_NOTIFICATIONS"/>` on my `AndroidManifest.xml`, I expected to use `PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS` but `POST_NOTIFICATIONS` is `undefined` and is named `POST_NOTIFICATION` instead.

Every other Android permission is 1:1 in spelling except this one where it lacks `S`.

Not sure if this is a welcome change since this can be breaking. Or maybe we can include both with and without `S` to not be a breaking change. Or just keep it as is and close this PR.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[Android] [Changed] - Rename `POST_NOTIFICATION` to `POST_NOTIFICATIONS`

Pull Request resolved: #35533

Test Plan: `PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS` should not be `undefined`.

Reviewed By: cortinico

Differential Revision: D41705866

Pulled By: philIip

fbshipit-source-id: a0328b174f0196421565f0cd2b2f1eb509428553

# Conflicts:
#	packages/rn-tester/js/examples/PermissionsAndroid/PermissionsExample.js
cipolleschi pushed a commit that referenced this pull request Jan 30, 2023
Summary:
After adding `<uses-permission android:name="android.permission.POST_NOTIFICATIONS"/>` on my `AndroidManifest.xml`, I expected to use `PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS` but `POST_NOTIFICATIONS` is `undefined` and is named `POST_NOTIFICATION` instead.

Every other Android permission is 1:1 in spelling except this one where it lacks `S`.

Not sure if this is a welcome change since this can be breaking. Or maybe we can include both with and without `S` to not be a breaking change. Or just keep it as is and close this PR.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[Android] [Changed] - Rename `POST_NOTIFICATION` to `POST_NOTIFICATIONS`

Pull Request resolved: #35533

Test Plan: `PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS` should not be `undefined`.

Reviewed By: cortinico

Differential Revision: D41705866

Pulled By: philIip

fbshipit-source-id: a0328b174f0196421565f0cd2b2f1eb509428553
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
After adding `<uses-permission android:name="android.permission.POST_NOTIFICATIONS"/>` on my `AndroidManifest.xml`, I expected to use `PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS` but `POST_NOTIFICATIONS` is `undefined` and is named `POST_NOTIFICATION` instead.

Every other Android permission is 1:1 in spelling except this one where it lacks `S`.

Not sure if this is a welcome change since this can be breaking. Or maybe we can include both with and without `S` to not be a breaking change. Or just keep it as is and close this PR.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[Android] [Changed] - Rename `POST_NOTIFICATION` to `POST_NOTIFICATIONS`

Pull Request resolved: facebook#35533

Test Plan: `PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS` should not be `undefined`.

Reviewed By: cortinico

Differential Revision: D41705866

Pulled By: philIip

fbshipit-source-id: a0328b174f0196421565f0cd2b2f1eb509428553
@cipolleschi cipolleschi mentioned this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants