-
Notifications
You must be signed in to change notification settings - Fork 321
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
feat(android, ios): add 5G support #130
base: master
Are you sure you want to change the base?
Conversation
How do we get pull requests to get merged in master branch? |
Any news about when to release this fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR.
I've added some notes, if you have any questions feel free to ask.
Noting that due to the added dependency of androidx
this will be considered a breaking change for the plugin. This isn't a bad thing, just simply a note for maintainers, where this plugin will require at least cordova-android@9 (with AndroidXEnabled
preference enabled).
Thank, these days are a little problematic for me but I will check and resolve all as soon as possible |
Sorry was impossible to me to do this changes before. I update all as you requested and tested in my local. If you have more feedback or changes to me add let me know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the requested changes.
One last thing, the package-lock
change should also be reverted, if possible. package-lock updates are typically done in their own independent PRs. This way if we ever had to revert this PR in the future, the revert won't be touching the package-lock.
As for the code changes, I've had a glance over and everything looks ok to me. I've approved the test actions to run and hopefully I can find some time tomorrow night to give it a test in an emulator.
You are right. Sorry I didn't realized about the changes on the package-lock.json. I will do the revert now |
what about this? |
APIs are only available in 14.1, not in 14.0 Co-authored-by: David Boho <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@ZumelzuR On my iPhone 13 Pro Edit: May you will use a more reactive form (with a nil check): static NSString *radioAccessNameIn(CTTelephonyNetworkInfo *networkInfo) {
if (@available(iOS 13.0, *)) {
if (networkInfo.currentRadioAccessTechnology == nil && networkInfo.dataServiceIdentifier) {
return [networkInfo.serviceCurrentRadioAccessTechnology objectForKey:networkInfo.dataServiceIdentifier];
}
}
return networkInfo.currentRadioAccessTechnology;
} |
Thank you, ok yes I will do some testing on my devices and come back with the fix and an update. |
Done, I added the fixes at b65a783 |
I got an error network-information/CDVConnection.m:63:94: expected ';' at end of declaration |
@ZumelzuR I see IOS Test suites are failing for this PR |
For fix this I should try to simulate a 5g connection in a ios12 device and check if is working? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just nitpick changes to cleanup the PR.
- Cordova uses a 4-space indention.
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
Co-authored-by: エリス <[email protected]>
some update on this? |
closed/re-opened the PR as the test results expired and wasn't available. Assuming that the tests passes I'll do one last row call and I'll merge if there are no objections in the next ~24 hours. |
} | ||
#if __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_14_1 | ||
else if (@available(iOS 14.1, *)) { | ||
if ([currentRadioAccessTechnology isEqualToString:CTRadioAccessTechnologyNRNSA]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the preprocessor macro doesn't appear to be working as intended here cause iOS 13 tests is failing on this line due to CTRadioAccessTechnologyNRNSA
not being available.
/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmp-3401-5V69QdTKGZQj/platforms/ios/HelloCordova/Plugins/cordova-plugin-network-information/CDVConnection.m:89:76: error: use of undeclared identifier 'CTRadioAccessTechnologyNRNSA'
if ([currentRadioAccessTechnology isEqualToString:CTRadioAccessTechnologyNRNSA]) {
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@breautek this error not make much sense for me, I mean is in a block of code that if the version is iOS14 will try to access.
Also I read this error is related with the xcode
dcloudio/native-docs#30
https://stackoverflow.com/questions/71585983/use-of-undeclared-identifier-ctradioaccesstechnologynr
Any suggestions? I have the last version of xcode and I will try to run it in iOS 13 simulator and check if I can reproduce
} | ||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is also causing a syntax error...
var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmp-3401-5V69QdTKGZQj/platforms/ios/HelloCordova/Plugins/cordova-plugin-network-information/CDVConnection.m:113:1: error: extraneous closing brace ('}')
}
Platforms affected
Motivation and Context
closes #125
Description
Testing
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)