feat: added translations functionality on adapter level #2862
Conversation
|
This PR is still wip |
|
Yes I saw it. Thought I could attach my comments now too to eventually suggest additions. If this is not OK, sorry for it. |
99 % of adapters won't use translations in the backend, the translate function will only be available if specified via adapter options. Likely these will also be different translations as in the frontend. Also frontend has some suboptimal placeholder logic which will be optimized for the backend. admin/i18n as stated in the folder structure is meant for admin so some might just want to place it in i18n of the root package or maybe build/i18n, we can recommend some but shouldn't really care as paths need to be passed down. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Please consider that using a different place then admin/i18n causes additional effort at weblate and will require Adaption to adapter-dev translation Tool. In Addition I would suggest to configure additional translation directory at io-package.json instead of code as this location coukd be accessed by repo-checker and adapter-dev to retrieve location of files if such a configurationnis really required. |
There was a problem hiding this comment.
Hm ... ok the question on directory structure and such migth get interesting.
Lets's asssume an adapter that uses this also has translated Admin text. These are usually in i18n/lang/translations.json ... two sources in one adapter is challenging for weblate. Also true.
As soon as we come to the point that "Devicemanager texts" are the relevant ones translated here too then we are again on "text for the UI".
So the idea to have multiple paths is great, so devs can choose what to do and how, so main question is the structure ... should we switch to the current standard or support both?
| let translation = this.translations.get(key); | ||
|
|
||
| if (translation === undefined) { | ||
| throw new Error(`No translation found for key "${key}"`); |
There was a problem hiding this comment.
Better return the key as it is usually done in the UIs? Throwing errors might be problematic
There was a problem hiding this comment.
The rule should be
Return translatio
Return english trabnsation if no matching translation exists
Return key if no translation exusts at all.
An exception should not be raised
There was a problem hiding this comment.
The translations have fallbacks when loaded, either provide full translations or no, with this approach only translations for one language are in memory at time. It is a new method it does not replace FE translation methods in any way and keeping it clean is preferred from my side. A fail fast approach lets the developer know in time that used methods are wrongly applied.
There was a problem hiding this comment.
In my oppinion an exception should NEVER be raised here. Its likely that some places exist in code which are not really executed during tests - especially as adapetrs in reality are not tested with all possible exceptional paths. (I nknow thats not good practise but its reality for most adapoters in my oppinion). Crashing the adapter at such places would be inappropiate for users. So every adapter trying to avoid such situations would / should encapsolate the translate call into one central try/catch routine to dismiss the exeption and return the key. This could be done centrally too.
If a feedback is desired adapter code could log a warning that no translation for key xxx is available. This would be a feedback to dev without crashing an adapter.
| let content: string; | ||
|
|
||
| try { | ||
| content = await fs.readFile(path.join(configDir, `${lang}.json`), { encoding: 'utf8' }); |
There was a problem hiding this comment.
do we not have fsextra here, so we can use better methods?
There was a problem hiding this comment.
We have, however as now (I think) all of the methods we use have native promise based methods I thought of preferring these in the long run.
|
Supporting more than one Costa additional effort at Tools. But I can life with that if there are other benefits. BUT total freedom causes really problems and I do not see the Benefiz if one User/i18n thenext /myText/i18n etc. In addition we should consider to enforce i18n/language/translations.jsonbstructure. current Flexibilität to use either directory Basedow Strukturen or durect language.json file in i18n have already caused problems at weblate. As long as there is no clear Benefiz we should require fixed baming and lication at least by spec. This could be lassen later if there is a real need. Otherwise checker code and trabslation Tools will gave much more effert to solve. Last but not leise please ecaluate the Suggestion to put any config containing paths into io-pacjage as Tools cannot access this Information insode Adapter code but Adapter code can easily access data withon io-package.json |
Isn't weblate configured for each project separately anyway?
The benefit is that files are a lot smaller and the backend approach does not support both, as mentioned in the docs. |
Yes but ONE weblate component can only mange ONE file location. Currently one component is configured per adapter. If there are multiple locations multiple components must be configured (with slightly different names) and the second / all additional components must be configured special as they must not access the original git repo but the cloned one at weblate. So it's possible, but additional effort and one can loose overview which weblate component manages which translations. adapter-dev cannot handle other places currently (but can be extended of course if someone has time ans access)
Well size might be an argument to split frontend and backend. But this is no argument (in my oppinion) to allow more then 2 places (one backend, one frontend). Besides I guess that one of the primary reason for the original request was that sendTo from Adapter can return localized text to display at config. But this would be frontend text again although generated at backend. :-). Hope that noone will try to translate logs (- This should be mentioned at documentation I suggest -) Any oppinion to my suggstion to specify the location of the files at io-package.json so that Tools (checker, adapter-dev) can locate them without guessing or scanning and assuming things? |
|
Why did you close? |
As it seems like just Frontend translations are required and not backend like stated in the issue, please open up an issue for adapter-react-v5 to encapsulate the i18n functionality https://github.com/ioBroker/adapter-react-v5/blob/main/src/i18n.ts then this can simply be reused on the frontend translations from everywhere and the 5 adapters which will need this can require the module separately. Also it seems here are other misunderstandings, the config via adapter options is used to determine on type level if the translate method is available or not also discussions about throwing errors, we have added a lot of code that now throws since 2-3 controller versions on misusage of methods and I don't want to discuss here general practices again and again. Also I don't have any time to look into Weblate and adapter dev portal nor using it, so just please re-use adapter-reacts translation functions. |
Link the feature issue which is closed by this PR
Implementation details
Provide a method to translate via
adapter.translate(key)this needs the developer to pass the paths to the i18n directory or directories viatranslationDirectoriesas adapter options.Fallback is always
enif no folder for active language is provided.For more details see added docs.
Tests
Documentation