Skip to content
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

Option to disable method swizzling of NSLocalizedString #163

Closed
chrispomeroyhale opened this issue Jan 14, 2022 · 2 comments
Closed

Option to disable method swizzling of NSLocalizedString #163

chrispomeroyhale opened this issue Jan 14, 2022 · 2 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@chrispomeroyhale
Copy link

Is your feature request related to a problem?
My teammates have communicated concern about the use of Objective-C method swizzling of NSLocalizedString. The SDK unexpected changes functionality of something it doesn't own (an Apple API), and we may unknowingly be integrating with another SDK that also use swizzling.

Because our app is only interested in Over-the-Air for the time being, we'd like the option to disable swizzling. As I understand, swizzling is only a requirement of the optional Real-Time Preview feature, and for improved functionality of the also optional IntervalUpdates feature. Perhaps we will be interested in enabling Real-Time Preview for non-production builds with swizzling at a later date.

Describe the solution you'd like
If the SDK replicates Apple's, can we make Localization.localizedString(for:) public so that developers can call it directly? And then include a config option that conditionally disables CrowdinSDK.swizzle() and CrowdinSDK.swizzleControlMethods().

@chrispomeroyhale chrispomeroyhale added the enhancement New feature or request label Jan 14, 2022
@andrii-bodnar andrii-bodnar added the help wanted Extra attention is needed label Jan 14, 2022
@serhii-londar
Copy link
Collaborator

@chrispomeroyhale Thanks for the suggestion!
First of all, we need swizzling for comfortable usage of our SDK. After adding our SDK you don't need to change all NSLocalizedString() methods with some method from our SDK, it will automatically use localized strings from Crowdin. I think lots of people use extensions and it's not a big problem, but generally, our implementation should save the developer's time.
Regarding another SDK that also uses swizzling for the same methods, it could be detected during development, as our functionality might stop working. I think it's a good point to investigate.
I need some time to check if we can disable swizzling and have a fully working Over-The-Air feature.

@chrispomeroyhale
Copy link
Author

chrispomeroyhale commented Jan 29, 2022

Thanks for the reply @serhii-londar! I am not suggesting removing swizzling for all clients, I agree that it makes the SDK a drop-in replacement for most use cases.

However, for historical reasons our application already has a wrapper around our calls to NSLocalizedString. There is talk about removing calls to NSLocalizedString entirely because we have business rules where the server determines the current language.

Another issue that came up in #165 is that method swizzling appears to be interfering with testing. For instance, if I just wanted to test LocalizationProvider independently, I would think there should be no reliance on global state? Unfortunately I don't have enough familiarity with this code base and the clients you serve, so I am leaning on you to help us figure out what the best approach may be.

@andrii-bodnar andrii-bodnar closed this as not planned Won't fix, can't repro, duplicate, stale Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants