Closed ami-aman closed 1 year ago
Pull request title looks good 👍!
If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is 1.0.0
. If this pull request gets merged in, the next release of this project will be 1.0.1
. This pull request is not a breaking change.
All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time.
My vote is to make a bug fix for the native SDKs that returns 'NotDetermined' instead of 'Notdetermined' to fix this issue.
Levi, based on your PR you are using .capitalized
here, the enum returns Notdetermined
and NOT NotDetermined
.
Hence, native module returns Notdetermined
but reference property in javascript returns NotDetermined
.
Does this answer your question ?
My original code review feedback was not very helpful I am realizing.
My feedback is suggesting that instead of fixing this bug in the typescript/javascript layer of the code, we fix the bug in the Swift/native layer. This solution feels error-prone to me because of inconsistent naming. Instead of having both NotDetermined
and Notdeteremined
strings being used in the code base, I like the solution of only having NotDetermined
strings being used in the code. If we were to fix the bug in the Swift/native layer, we would no longer need to handle Notdetermined
strings from occurring in the code base.
To demonstrate this idea, I did create this PR. That new PR is not tested, but it is ready for a review to give feedback if that solution is preferred over this one.
I think the solution proposed by Levi is the right fix for this. Notdetermined
string was due to the bug, and we should fix the string and have NotDetermined
instead.
Although I am fine with the change, our clients who already use "Notdetermined" will experience a code break. But if you all feel that we should still go ahead and make the change then I am fine with that as well. In that case, feel free to discard this PR.
Related to GH issue https://github.com/customerio/customerio-reactnative/issues/115
PushPermissionStatus.NotDetermined is
NotDetermined
but the package receivesNotdetermined
from native modulesThis PR fixes the mismatch