search

briankabiro / react-native-get-sms-android

React Native module to get messages on an Android device
MIT License
137 stars 68 forks source link

Remove `uses-permission` from AndroidManifest.xml #35

Closed ghost closed 5 years ago

ghost commented 5 years ago

Fixes #34.

briankabiro commented 5 years ago

Great job. This looks good to me. 👍 Will wait for Mike's approval before we merge it.

mikehardy commented 5 years ago

Yeah - I'm okay with this philosophically, I think it's a good change

But it's a breaking change as integrating it might cause existing installs to suddenly stop working with permission errors. For the record I am pro-breaking change just with some help for devs

I feel it should be accompanied by a bump to version 2.0 and a "migrating" section in the README that outlines what changed and why (this PR has great text for that already) and what exactly needs to be done (again, I think the PR does a great job already).

Not trying to create busy-work, I am literally picturing about 7 lines in the README.md - it is just that you know that feeling when a library you depend on either breaks something with a minor release (arrrggghhh!!) or you see a major release, go to the site to see what happened and either blow an hour reading stuff (booo) or see that magical "migrating from version N to version N+1" section (sound of angels singing)

I'd like to be in the "angels singing" section of modules :-)

ghost commented 5 years ago

Yeah. Totally agree... WIll add a upgrade section in README.

@mikehardy When are you planning to release the new version? Just curious so that I can upgrade it to my app too.

mikehardy commented 5 years ago

@sulaysumaria not me man, @briankabiro is the boss here, I'm just helping out because I use the module and like spreading my effort upstream too :-)

briankabiro commented 5 years ago

Hey @sulaysumaria. We(I) usually do a release after merging a PR. Once we have the README updated, we can get this merged and do a release.

ghost commented 5 years ago

@briankabiro, Updated the README. Maybe you can review it once if it is good enough.

mikehardy commented 5 years ago

readme change looks good and then I just thought hey! does the example have this in the manifest so it will continue to work? I checked and it does, so it should still run out of the box. LGTM...

ghost commented 5 years ago

Yeah.

I just noticed the example is missing the deleting messages exmaple.

On other note Maybe we should add all this points to check in a CONTRIBUTING.md file. Like what all things to perform when contributing. To mention a few:

Something like a checklist?

mikehardy commented 5 years ago

I love the idea of a checklist personally, markdown has it and it works well in other projects I use. I would just put it in .github folder as a template like so https://github.com/react-native-community/react-native-device-info/blob/master/.github/pull_request_template.md

As for the example not including delete that's because delete is basically forbidden on android these days. You may only do it if you are the default SMS app on the system, which is a very strong position for an app to occupy - if you are doing that, you'll know how to do delete I think ;-). Could be worth a comment but for me, trying to get the example to demonstrate that (ask for permission to be the default SMS app etc) was more than I wanted to do when I was working in there...

ghost commented 5 years ago

Okay. So unless you set your app to default SMS App, there is no way to delete SMS. That's something new for me. 😄

I would also like the checklist. Maybe we can include that with the 2.0 release.

mikehardy commented 5 years ago

https://developer.android.com/about/versions/kitkat.html#44-sms-provider - there is the reference - it is as of Android 4.4 (API19, which is almost completely obsolete meaning almost no one can delete sms now, unless they are the default provider) but as I read that changelog maybe this package could implement a listener for SMS_RECEIVED intent on the Android side and EventEmitter on the react-native side, allowing for javascript listeners to hook in and be notified of new SMS

I am not volunteering but it seems like it would be fit in this module :-)

As for the PR template, I am not authoritative and would not want to stop this PR from merging but that is an easy stand-alone PR if you have the time to submit it...

ghost commented 5 years ago

Okay. In another PR then.

briankabiro commented 5 years ago

Hi, going to take another look at this and see whether we can get it merged.

briankabiro commented 5 years ago

Hey, @sulaysumaria. Apologies for the late reply.

I think there's a merge conflict in the README file from the merged changes in #40.

Could you be kind as to make the change?

ghost commented 5 years ago

@briankabiro , this PR is actually too old and I am no longer using react-native... Anyone else who wants this change to be merged can look at this...

Btw, I switched to flutter and its awesome...

briankabiro commented 5 years ago

Haha, I understand. I can make relevant changes and merge it.

briankabiro commented 5 years ago

Going to close this as I've opened a duplicate PR here. #41

Thanks, @sulaysumaria. I'm also going to get started on creating a changelog for the project.