Closed faizplus closed 6 months ago
Latest commit: e2d541db8a16895f500bc948b6fa45a5b2f52ff0
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
You need to add the
ACCESS_NETWORK_STATE
permission to yourAndroidManifest.xml
file. Here's how you can do it:
@faizplus I think it's possible to add the required permission to this library's own Android Manifest file, instead of requiring the user to update the apps Android Manifest manually. The library's and app's manifests are merged according to the behaviour described in the Android Docs.
An example of this can be found in the Amplitude React Native SDK, where they specify the ACCESS_COARSE_LOCATION
permissions in their AndroidManifest file
(Note: Although I believe this should work, I've never tried it myself)
Thanks @robwalkerco for your input. I tried adding ACCESS_NETWORK_STATE
to this library's own Android Manifest file, and this is working perfectly fine. We can safely say that the user is not required to update the apps Android Manifest manually.
Let me know if this works for you, I will update this pr.
@carbonrobot I have updated the code to include ACCESS_NETWORK_STATE
in the package itself so that the user don't have to add it manually as suggested by @robwalkerco. Now this PR is not introducing any breaking changes.
Currently this library requires no additional user permissions. After this change, all consuming applications will require the network access permission for only Android apps.
Background:
Existing user code needs to check for network errors by parsing the error message:
try {
await refreshToken();
setIsLoggedIn(true);
} catch (error) {
if (message.includes('Connection error') {
console.log('Connection error');
}
}
Internally, we could wrap this error up, by moving the above code into the module and exposing an error code for the same. The user code improves a little, but not by a whole lot.
try {
await refreshToken();
setIsLoggedIn(true);
} catch (error) {
if (error.code === 'network_error') {
console.log('Connection error');
}
}
We could also take the approach in the PR, and request android.permission.ACCESS_NETWORK_STATE
permissions, but the result is the same on the user code side and only works on Android.
try {
await refreshToken();
setIsLoggedIn(true);
} catch (error) {
if (error.code === 'network_error') {
console.log('Connection error');
}
}
Several questions:
Original issue #861
No expert here, but I thought I'd chime in as well because I'm also not sure if the permission is necessary, as suggested by @carbonrobot . I concur that intercepting a network error by parsing the error message is not great, but I'm not sure if adding a permission to check for a network error strikes a better balance.
Unless there are any obvious reasons why parsing the error message in the library's code isn't viable option, I would go with that, and then expose to users a better way to intercept it, as you have already done in this PR.
We have decided internally to not handle this within our library and leave checking the network status up to consumers. Will add documentation in a future PR to help advise users on how to check status in different ways using some of this code as an example/guide.
Fixes #861 for Android only
Description
This pull request adds error handling for network connectivity issues in the
refresh
and other function of this React Native module. Previously, therefresh
or other function did not check for network connectivity before performing the token refresh, leading to potential issues when the device was not connected to a network. Introducednetwork_error
error code for all functions.Steps to verify
refresh
function like below snippet.It should have
network_error
inerror.code