Skip to content

Rename POST_NOTIFICATION to POST_NOTIFICATIONS - #35533

Closed
dcangulo wants to merge 1 commit into
react:mainfrom
dcangulo:chore/fix-permission-spelling
Closed

Rename POST_NOTIFICATION to POST_NOTIFICATIONS#35533
dcangulo wants to merge 1 commit into
react:mainfrom
dcangulo:chore/fix-permission-spelling

Conversation

@dcangulo

@dcangulodcangulo commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

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-botfacebook-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-botreact-native-bot added the Platform: Android Android applications. label Dec 2, 2022
@analysis-bot

analysis-bot commented Dec 2, 2022•
edited
Loading

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a7,102,159-35
androidhermesarmeabi-v7a6,470,950-27
androidhermesx867,520,368-33
androidhermesx86_647,378,725-34
androidjscarm64-v8a8,969,961+14
androidjscarmeabi-v7a7,701,357+10
androidjscx869,032,670+7
androidjscx86_649,510,150+5

Base commit: 436da18
Branch: main

@analysis-bot

analysis-bot commented Dec 2, 2022•
edited
Loading

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: 436da18
Branch: main

@pull-bot

pull-bot commented Dec 2, 2022

Copy link
Copy Markdown

PR build artifact for fd3cf8a189c51a26ef23cb169674de8d229f2dd4 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

pull-bot commented Dec 2, 2022

Copy link
Copy Markdown

PR build artifact for fd3cf8a189c51a26ef23cb169674de8d229f2dd4 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.

cortinico
cortinico reviewed Dec 2, 2022
View reviewed changes
Comment threadLibraries/PermissionsAndroid/PermissionsAndroid.js Outdated

@cortinicocortinicoDec 2, 2022

Copy link
Copy Markdown
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)

@kelsetkelsetDec 2, 2022

Copy link
Copy Markdown
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

@cortinicocortinicoDec 2, 2022

Copy link
Copy Markdown
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

@kelsetkelsetDec 2, 2022

Copy link
Copy Markdown
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.

@kelsetkelsetDec 2, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@cortinicocortinicoDec 2, 2022

Copy link
Copy Markdown
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

@dcangulodcangulo mentioned this pull request Dec 2, 2022
Merged
@pull-bot

pull-bot commented Dec 2, 2022

Copy link
Copy Markdown

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

pull-bot commented Dec 2, 2022

Copy link
Copy Markdown

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

dcangulo commented Dec 2, 2022

Copy link
Copy Markdown
ContributorAuthor

@cortinico Restored to not be a breaking change.

@facebook-github-bot

facebook-github-bot commented Dec 3, 2022

Copy link
Copy Markdown
Contributor

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

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

facebook-github-bot commented Dec 9, 2022

Copy link
Copy Markdown
Contributor

@philIip merged this pull request in 910a750.

kelset pushed a commit that referenced this pull request Dec 13, 2022
@dcangulo@kelset
RenamePOST_NOTIFICATIONtoPOST_NOTIFICATIONS(#35533)
3fb93e1
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
@dcanguloRiccardo Cipolleschi
RenamePOST_NOTIFICATIONtoPOST_NOTIFICATIONS(#35533)
b5280bb
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
@cipolleschicipolleschi mentioned this pull request Aug 16, 2023
Merged
@cipolleschicipolleschi mentioned this pull request Sep 21, 2023
Merged
@cipolleschicipolleschi mentioned this pull request Oct 11, 2023
Merged
This was referenced Nov 1, 2023
Merged
Closed
@cipolleschicipolleschi mentioned this pull request Dec 19, 2023
Merged
This was referenced Jan 8, 2024
Merged
Merged
Merged
@cipolleschicipolleschi mentioned this pull request Mar 6, 2024
Merged
@cipolleschicipolleschi mentioned this pull request Apr 5, 2024
Closed
@cipolleschicipolleschi mentioned this pull request Apr 26, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cortinicocortinicocortinico approved these changes
+1 more reviewer
@kelsetkelsetkelset left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: AndroidAndroid applications.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@dcangulo@analysis-bot@pull-bot@facebook-github-bot@cortinico@kelset@react-native-bot