Closed liamnichols closed 1 week ago
Thank you for your detailed writeup!
This looks to be a big change for a specific use case. The first thing that comes to mind: Can this be written as an extension instead?
The StringResource types in Rswift are created specifically so that extensions can be written in third-party libraries. They contain should all the data necessary to write something like:
extension StringResource {
func callAsFunction(loadingStrategy: LoadingStrategy) -> String {
...
}
}
Thanks @tomlokhorst! Yeah I agree that it's quite a big change, and you're right that it likely is too much for such a specific use case.
Can this be written as an extension instead?
This is a great question. I was caught up thinking of how to integrate it directly so that we could continue using the API directly like we had done previously, but I didn't think about extending the functionality instead.
The first thing that comes to mind is that I don't want to have to update each of my call sites to inject loadingStrategy
, but I will play around to see if there is a way to overload the existing callAsFunction
methods and run with a custom implementation.
Thanks again for the review/suggestion π
Closing this, because this feature can be implemented via an extension instead of built into the library.
Thanks again for the detailed proposal and PR!
π
Background
While best practices encourage apps to leave localisation down to the system, there are still many scenarios where this just doesn't cut it. The main use-case is where an app needs to override the system (or app specific) language selection and display content in a different languages, but also there are some limitations in how
NSLocalizedString(_:tableName:bundle:value:comment:)
handles missing translations in some languages that developers want to work around themselves.While using non-system languages is a use case that is officially supported by R.swift via the
preferredLanguages
APIs, it is not currently possible to gain greater control over howNSLocalizedString
is used or to bypass it entirely.Goals
preferredLanguages
lookupNSLocalizedString(_:tableName:bundle:value:comment:)
for more advanced use casesProposal
Currently in R.swift, the source of the localization is determined by
source
property defined onStringResource
. Thesource
is resolved when accessing a generatedStringResource
property and it describes how theString.init(key:tableName:source:developmentValue:locale:arguments:)
initializer should load the string:https://github.com/mac-cain13/R.swift/blob/5721c1a948429232718fcbea3eb6132675192a72/Sources/RswiftResources/Integrations/StringResource%2BIntegrations.swift#L12-L27
This approach has been great for catering to the
preferredLanguages
API, however it is no longer sufficient for the second goal in this proposal. Instead, it is proposed thatStringResource.Source
is removed in a breaking change and replaced with a new approach for loading localized string values.While such a change would be breaking, it would only be breaking to the RswiftResources API. Code generated by
rswift
will remain backwards compatible, although some of the existing methods will be deprecated and removed in a future version.To replace
source
, a new type should be introduced:LoadingStrategy
has a similar responsibility toSource
however it allows for complete control over converting aStringResource
toString
via thecustom
case. The enum more directly describes the approach used to load a localized string and defers any actual work until theString
needs to be loaded in the updatedString.init(key:tableName:bundle:loadingStrategy:developmentValue:locale:arguments:)
initializer.Changes to the interface of generated
_R
types:And changes to the interface of nested types such as
localizable
:A consumer of R.swift can then leverage this new point of customisation similar to the example below:
While this is a non-breaking change at the generated source level, there are breaking changes within the RswiftResources module:
StringResource.Source
enum is removedStringResource.source
property is removedStringResource{1-9}
initializers now requirebundle
andloadingStrategy
String.init(key:tableName:source:developmentValue:locale:arguments:)
initailizer replaces thesource
argument withbundle
andloadingStrategy
Additional Questions
string(preferredLanguages: ...)
methods in favour of string(loadingStrategy: .preferredLanguages(...))? It's a bit more bloated, but also my guess is that people would use these kind of methods are also likely encouraged to define their own
R` interface anyway.Alternatives Considered
Add
RswiftLocalizedString
extension pointInstead of refactoring/replacing
Source
withLoadingStrategy
, we could keep it exactly how it is and instead provide a hook into replacing calls toNSLocalizedString
as demonstrated in #841.Add
StringResource.Source.custom
enum caseInstead of introducing an entirely new
LoadingStrategy
type, we could just add the.custom
case to the existingSource
enum however there are some reasons why I preferred to not take this approach...Choosing the correct
Source
for thepreferredLanguages
array currently requires us finding the correctBundle
prior to initializing theStringResource
. In an ideal world, this isn't the right place since we shouldn't need to do any 'work' until the point that the user actually wants a localizedString
. In reality, you are most likely to create theStringResource
at the same point of invokingcallAsFunction
to produce the localizedString
so this has never really been a huge deal, but when experimenting with additive changes, it felt like theSource
Β enum really wasn't the right type.Next Steps
Firstly, thanks for taking the time to read though this proposal π I understand that it's quite a complex change to support such a complex use case but it's currently a blocker for us to migrate to v7 and I would love for it to officially be supported in a way that keeps the code in this project at a high standard π
I've included an implementation of the main proposed implementation in #842 and also have #841 for the first alternative. What I haven't done yet is look more into the second alternative.
My suggestion for next steps would be to: