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

i18n.updateTranslations does not respect t-params bindings #273

Open
rluba opened this issue May 29, 2018 · 13 comments · May be fixed by #330
Open

i18n.updateTranslations does not respect t-params bindings #273

rluba opened this issue May 29, 2018 · 13 comments · May be fixed by #330
Milestone

Comments

@rluba
Copy link
Contributor

rluba commented May 29, 2018

I'm submitting a bug report

  • Library Version:
    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 because i18n.updateTranslations does not respect t-params bindings.

Template-Snippet

<span t="daysLeft" t-params.bind="{count: 25}">{{count}} days left</span>

(daysLeft translates to the same as the html content: {{count}} days left)

I’ve stepped though aurelia-i18n when it happens:

  1. The span’s content is initially translated to days left because params.value is not yet set when t.bind() fires.
  2. Immediately after that, the content is updated to 25 days left when the t-params.valueChanged handler fires with the correct params value.
  3. Then Base18N’s attached handler fires and calls i18n.updateTranslations(…). But this method calls updateValue(…) without passing the parameters from t-params, causing the span content to be overwritten again with days left.

Expected/desired behavior:
Content should read 25 days left instead of days left.

Additionally, it seems extremely wasteful that the content of every translated element is overwritten three times during initial loading.

  1. Why does TCustomAttribute not postpone the translation when params is present but its value is still undefined?
  2. Is it really necessary to call i18n.updateTranslations in BaseI18N.attached when the attributes already take care of the translation when they are bound?
@zewa666
Copy link
Member

zewa666 commented May 30, 2018

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.

@rluba
Copy link
Contributor Author

rluba commented Jun 1, 2018

I just stopped using BaseI18N to fix 2. I can still switch languages at runtime so I really don’t see the point of BaseI18N.

I’ve used au new to create a minimal example project that reproduces the "missing parameter" issue.

@zewa666
Copy link
Member

zewa666 commented Jun 2, 2018

thanks for the repo, but I can't get it to run. Is it CLI based? The tasks build/run report a missing configureEnvironment from ./environment. Could it be you haven't checked in everything?

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.

@rluba
Copy link
Contributor Author

rluba commented Jun 2, 2018

Could it be you haven't checked in everything?

Sorry, my fault. One of the task files got excluded by .gitignore. I fixed it.

Another reason is that t-params are retrieved lazy, as they might not be present.

I don’t think that’s correct. Yes, it’s lazy, but TCustomAttribute explicitly checks in bind whether a t-params exists before performing the initial translation.

That could be the actual use case, to set the param to undefined in order to fall back to the default translation.

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 t-params to undefined as a valid use-case? It sounds much more likely to be a programming error if t-params is present, is bound to a value, and the value is undefined. t-params is supposed to be bound to an object. If you want the "default translation" (i.e.,"no count" behavior), you could just bind it to an empty object or one with an undefined count.

@zewa666
Copy link
Member

zewa666 commented Jun 4, 2018

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 default translation was just an example.

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.

@rluba
Copy link
Contributor Author

rluba commented Jun 4, 2018

Params are checked thats right, but not their value.

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?"

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.

Yeah, but in that case my suggested solution would work: If t-params is present but the value is undefined, then postpone the translation until the valueChanged handler fires. This would fix the "undefined parameters" warning and avoid translating everything twice.

… I personally wouldn't recommend using anymore

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.

@zewa666
Copy link
Member

zewa666 commented Jun 4, 2018

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.

rluba added a commit to rluba/i18n that referenced this issue Jun 5, 2018
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))
zewa666 added a commit that referenced this issue Jun 6, 2018
@zewa666
Copy link
Member

zewa666 commented Jun 6, 2018

hmm there might be a fix for having the updateTranslations function also figure out params. I've created a branch updatetranslations-params which includes the fix and built files. If you could give this a try npm install aurelia/i18n#updatetranslations-params that would be helpful

@zewa666
Copy link
Member

zewa666 commented Jun 30, 2018

Ping @rluba. Did you have a chance to try the mentioned fixes above?

@rluba
Copy link
Contributor Author

rluba commented Jul 23, 2018

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 t attribute.

@zewa666
Copy link
Member

zewa666 commented Aug 1, 2018

Thanks for checking it out @rluba. Yes I agree, still it's worth having the fix in the base.
Guess we can close this one or is there anything left from your side?

zewa666 added a commit that referenced this issue Aug 1, 2018
when updateTranslations is called for a element, it now passes on params

related issue #273
@rluba
Copy link
Contributor Author

rluba commented Aug 1, 2018

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!
This causes me to ignore all those warnings – making them useless and increasing the chance of missing a real mistake.

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.

@zewa666
Copy link
Member

zewa666 commented Aug 1, 2018

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

@zewa666 zewa666 added this to the Rewrite to TS milestone Aug 1, 2018
rluba added a commit to rluba/i18n that referenced this issue Dec 17, 2020
Fixes aurelia#273 caused by interpolating the value before its params were bound.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants