-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
i18n.updateTranslations does not respect t-params bindings #273
Comments
Actually the BaseI18N class altogether isnt the best Idea in retrospective as inheritance causes more troubles than good. Could you create a minimal sample reproducing the issue and share it as GitHub repo? With regards to timing and overall architecture I'm planing to do a full rewrite of the Plugin in TS for better support of both languages and also cleaning up the promise/setup nightmare that creeped into the lib. |
I just stopped using I’ve used |
thanks for the repo, but I can't get it to run. Is it CLI based? The tasks build/run report a missing There used to be a case for BaseI18N in the beginnings of the plugin, when the t attribute wasn't fully implemented via Aurelia custom attributes with signaling behavior. With regards to the Q1, yep it needs to be performed, even if the value is undefined. That could be the actual use case, to set the param to undefined in order to fall back to the default translation. Another reason is that t-params are retrieved lazy, as they might not be present. |
Sorry, my fault. One of the task files got excluded by .gitignore. I fixed it.
I don’t think that’s correct. Yes, it’s lazy, but
Yikes, I did not even consider that this could be allowed. Are you sure that supporting this edge case warrants doing every translation twice and logging warnings to console for every translation that contains a param? Why would you consider binding |
Params are checked thats right, but not their value. The usage of t-params being undefined can stem from forwarding values passed into custom elements via custom attributes, which haven't yet resolved to a value. The With regards to BaseI18N, yep I definitely see that it's using the ancient form of updateTranlsations, which clearly doesn't walk through the params provided. As said this was a very old piece of code, which I personally wouldn't recommend using anymore. Guess with future versions this will simply be deprecated as A, it's easy to create an overload for one self, and B the update isn't necessary anymore. Thx for updating the sample btw. |
That’s exactly what I mean when I said: "Why does TCustomAttribute not postpone the translation when params is present but its value is still undefined?"
Yeah, but in that case my suggested solution would work: If
Then we should definitely update the I18N documentation on the main site. That’s where I got the idea and others might stumble into the same trap. Everything past "The following example shows how a view model can be configured to update it's contents when the view is attached and every time a locale is changed" is unnecessary now as far as I can tell. |
I agree, would you mind creating a PR for the docs to state it the way you would see it? With regards to your proposed fix, this would change the behaviour of the current workflow. Everyone, for whatever reason, who depends on that undefined step, will have a breaking change, whereas the double translation is merely a optimisation issue. I agree this should be done, but it's likely to happen with the full rewrite of the plugin as mentioned in the begin, because with v2 we can do breaking changes and deprecations of features which don't make sense anymore, the BaseI18N class being one of them. We'll certainly keep this issue around, but I cant promise to tackle this new part before July due to private time schedules. |
The section about using `BaseI18N` was outdated: * `TCustomAttribute` already observes language change events and updates itself. * Base18N does not respect `t-params`, causing incorrect translations. See [issue 273 for details](aurelia#273 (comment))
hmm there might be a fix for having the |
Ping @rluba. Did you have a chance to try the mentioned fixes above? |
I finally got around to testing your fix: it seems to work – but I’d nonetheless suggest not to use BaseI18N since it does zero useful work on top of the |
Thanks for checking it out @rluba. Yes I agree, still it's worth having the fix in the base. |
when updateTranslations is called for a element, it now passes on params related issue #273
I‘d still strongly suggest to change the initial „undefined“ translation because it’s more than just inefficient: it logs a warning for every parameter, saying that the parameter is missing – even though it isn’t! But I understand that you might consider the change „breaking“. If you fix it in a „next major“ branch, I can start using it. Otherwise I‘d have to fork this repo for now. |
Yeah I think this is due to the highly "inefficient" way of initializing the whole plugin. Admittedly it was at the time due to me not fully understanding the async behavior. With the next major all of this definitely needs to be rewritten and the initial translation should be gone. We're currently building out our monorepo infrastructure for Aurelia, so as soon as that is set, I plan on starting the rewrite |
Fixes aurelia#273 caused by interpolating the value before its params were bound.
I'm submitting a bug report
2.2.0
Please tell us about your environment:
Operating System:
OSX 10.13.4
Node Version:
8.9.1
NPM Version:
5.8.0
JSPM OR Webpack AND Version
webpack 4.1.1
Browser:
all
Language:
ESNext
Current behavior:
Using a translation attribute with params on a VM that inherits from
BaseI18N
causes the params to be ignored becausei18n.updateTranslations
does not respectt-params
bindings.Template-Snippet
(
daysLeft
translates to the same as the html content:{{count}} days left
)I’ve stepped though aurelia-i18n when it happens:
days left
becauseparams.value
is not yet set whent.bind()
fires.25 days left
when thet-params.valueChanged
handler fires with the correct params value.Base18N
’sattached
handler fires and callsi18n.updateTranslations(…)
. But this method callsupdateValue(…)
without passing the parameters fromt-params
, causing the span content to be overwritten again withdays left
.Expected/desired behavior:
Content should read
25 days left
instead ofdays left
.Additionally, it seems extremely wasteful that the content of every translated element is overwritten three times during initial loading.
TCustomAttribute
not postpone the translation whenparams
is present but its value is still undefined?i18n.updateTranslations
inBaseI18N.attached
when the attributes already take care of the translation when they are bound?The text was updated successfully, but these errors were encountered: