fix: translate validation text inside validation functions

[tomzemp]

Implements LIBS-483

---

Description

Currently the translation strings in form validators are defined outside of the exported validator function. This means that the translation is a constant, generated when the module gets imported. This works generally as users have to change their selected language and then navigate to another app, but this does not work in the context of the login app where the language selection is on the page, and we do not want to have to force refresh in order for translations to flow through.

---

Known issues

• With regards to the implementation in the login app: the validation strings returned from the validators will now update language once the validation is rerun (e.g. this occurs on field toggling with react final form validation) (so the translation is still not as instantaneous as some of the other on page translations)

---

Checklist

• API docs are generated
• Tests were added
• Storybook demos were added

The changes here do not change any functionality, so just updating the existing tests to pass seems sufficient.

---

Screenshots

[dhis2-bot]

🚀 Deployed on https://pr-1253--dhis2-ui.netlify.app

[kabaros]

@tomzemp do you think this could have a visible performance implication, having to call i18n.t several times rather than once? I am thinking for example if we're using the validators inside a table or a loop .. I am also noting that we are still using a version of i18next from 5 years ago so might have its issues anyhow.

Being a bit extra-conservative, could we perhaps force reinitialising i18next in the case of the Login app?

[Mohammer5]

do you think this could have a visible performance implication, having to call i18n.t several times rather than once?

I don't think that this should be an issue, these are primitive functions with little computation, just grabbing values by key from an object.

If you're very worried, I suppose the code could be changed so that the translation function is only called when the translation is needed:

Before:

const boolean = (value) => {
    const invalidBooleanMessage = i18n.t('Please provide a boolean value')

    return isEmpty(value) || typeof value === 'boolean'
        ? undefined
        : invalidBooleanMessage
}

After:

const boolean = (value) => {
    if (isEmpty(value) || typeof value === 'boolean') {
        return undefined
    }

    return i18n.t('Please provide a boolean value')
}

[Mohammer5]

With regards to the implementation in the login app: the validation strings returned from the validators will now update language once the validation is rerun (e.g. this occurs on field toggling with react final form validation) (so the translation is still not as instantaneous as some of the other on page translations)

I don't think that this should block this PR from being merged as this seems to be an edge case. A simple fix would be to increment a key prop when changing the language to force a rerender (not entirely sure that'd work, but it should be very straight forward to implement)

[kabaros]

I don't think that this should be an issue, these are primitive functions with little computation, just grabbing values by key from an object.

it does a bit more than key lookup but none of it looks expensive to be fair, also none of it seems to have changed in the 5 years since we introduced that version of the library.

I guess my point is that if re-initialising i18next solves the login app use case, then maybe we can just do that (when the user changes language in the login screen) rather than changing the UI components here. I am not sure if that's feasible though with the wrapper we have around i18next - in that case, this solution looks good.

[Mohammer5]

it does a bit more than key lookup but none of it looks expensive to be fair, also none of it seems to have changed in the 5 years since we introduced that version of the library.

Ah yeah, fair enough. I didn't expect that much to be going on..

I guess my point is that if re-initialising i18next solves the login app use case, then maybe we can just do that (when the user changes language in the login screen) rather than changing the UI components here. I am not sure if that's feasible though with the wrapper we have around i18next - in that case, this solution looks good.

Yeah, worth a try I guess

[tomzemp]

@kabaros

do you think this could have a visible performance implication, having to call i18n.t several times rather than once?

Thanks. My initial reaction was similar to @Mohammer5 's and I thought this would basically be a negligible key/value lookup, but I did a basic test in an app to try to separate out the additional time associated with the i18n.t functionality vs. just using a stored translation:

    const myString = i18n.t('a string')
    let testMessage
    const upperLimit=1000000

    const startNoTranslate = new Date()
    for (let i=0;i<upperLimit;i++) {
        testMessage = myString
    }
    const endNoTranslate = new Date()

    const startTranslate = new Date()
    for (let i=0;i<upperLimit;i++) {
        testMessage = i18n.t('a string',{lng: 'fr'})
    }
    const endTranslate = new Date()`

Sample run for this was 8ms elapsed without the translation and 3343ms elapsed with translation (1 million iterations) (I'm on a slow machine today, though). That is significantly worse performance wise; I'm not sure, however, if this would actually be noticeable (e.g. I think the rendering of a million error messages would cause a time out, so within the context of how these would be used, the performance hit should hopefully not be noticeable). That said, I think @Mohammer5's proposal of only accessing the translation if there is a violation would make sense and would also get rid of most of the performance hit (assuming that we generally expect things to be valid).

With regards to the alternate approach of reinitializing i18next: I'm not sure I follow how this would work? I think the issue with these validation functions is that the translation is defined at the time of the module import (so if my language, were say Norwegian, when the module was imported, they will remain in Norwegian even if the i18next language changes until the module is re-imported, which I think will only occur when the page is refreshed?). That is, I'm not sure if we can do anything with i18next from the login app because the error message string is a constant in an already imported module? If we think it's not worth it to update the translation approach for the validators (because only the login app allows you to change language on the page), I could also just write a couple of custom validators for the login app to avoid this issue.

@Mohammer5

With regards to the implementation in the login app: the validation strings returned from the validators will now update language once the validation is rerun (e.g. this occurs on field toggling with react final form validation) (so the translation is still not as instantaneous as some of the other on page translations)

I don't think that this should block this PR from being merged as this seems to be an edge case. A simple fix would be to increment a key prop when changing the language to force a rerender (not entirely sure that'd work, but it should be very straight forward to implement)

Yeah. Good point. We can just force the rerender 👍; I guess it's just an extra step, but not a big deal.

[Mohammer5]

@tomzemp I assume @kabaros meant calling i18n.init() again with the right lng

[cypress]

Passing run #2895 ↗︎

0

583

0

0

0

Details:

fix: call i18n translate only on invalid value
Project: ui	Commit: 4815d113ab
Status: Passed	Duration: 08:34 💡
Started: May 11, 2023 3:13 PM	Ended: May 11, 2023 3:22 PM

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[tomzemp]

FYI. I updated logic to move the translation until after the error was identified (as suggested by @Mohammer5).

I will put this PR on hold, though, until we decide the overall approach for login app customization, as that may affect how we handle language selection. If we use web components for customization, the app may need to be refreshed when changing language, which would make this PR unnecessary.