core: improve Translator type for strict TypeScript compatibility

[lucas-koehler]

Fix #2528

• Replace overloaded Translator type with a generic conditional type
• The generic type allows inferring the return type from the handed over default message. I.e. a undefined defaultMessage results in string | undefined while a string default message results in string.
• Add createTranslator helper to allow implementation without type assertions under strictFunctionTypes and strictNullChecks.

[netlify]

✅ Deploy Preview for jsonforms-examples ready!

Name	Link
🔨 Latest commit	633368c
🔍 Latest deploy log	https://app.netlify.com/projects/jsonforms-examples/deploys/69d4b1326e651a0007a2645e
😎 Deploy Preview	https://deploy-preview-2562--jsonforms-examples.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[coveralls]

coverage: 82.91% (+0.06%) from 82.852% — lk/issues/2528-translator-typing-issues into master

[sdirix]

I understand the intention of the change but personally I would like to do a different approach.

The PR solves the typing issue by force-casting a simplified translator method to our own. However this means that a "wrongly" implemented translator which takes in a "string"-default parameter but still returns "undefined", is accepted by our official typing and used, without further checking, as if the requirement holds true.

I see two different approaches:

• Either extend "createTranslator" to not be a no-op but actually enforce the invariant given above, or
• Get rid of the fancy typing, get rid of createTranslator and just use the simplified type in all call sites. In some places we then slightly need to do a bit more checking but should be manageable.

What do you think?

[lucas-koehler]

@sdirix Thanks for the feedback!

While I generally like the narrowing effect of the generic type, I see your concern. I honestly did not think of the problem of implementation faults that "illegally" return undefined even if a default value is given.

• Either extend "createTranslator" to not be a no-op but actually enforce the invariant given above

That might be a nice idea. We can just return the default value in any case if the called translator returns undefined. We could even extend this to also apply to null for non-strict or JS environments. Is this what you meant?

• Get rid of the fancy typing, get rid of createTranslator and just use the simplified type in all call sites. In some places we then slightly need to do a bit more checking but should be manageable.

I'm not sure what exactly you mean with that. Do you mean simplifying the Translator type to the following?

export type Translator = {
  (id: string, defaultMessage?: string, values?: any): string | undefined;
};

[sdirix]

@sdirix Thanks for the feedback!

While I generally like the narrowing effect of the generic type, I see your concern. I honestly did not think of the problem of implementation faults that "illegally" return undefined even if a default value is given.

• Either extend "createTranslator" to not be a no-op but actually enforce the invariant given above

That might be a nice idea. We can just return the default value in any case if the called translator returns undefined. We could even extend this to also apply to null for non-strict or JS environments. Is this what you meant?

Yes, exactly. Our current default (error) translator however only checks for undefined not for null etc. I would keep it that way I think.

• Get rid of the fancy typing, get rid of createTranslator and just use the simplified type in all call sites. In some places we then slightly need to do a bit more checking but should be manageable.

I'm not sure what exactly you mean with that. Do you mean simplifying the Translator type to the following?

export type Translator = {
  (id: string, defaultMessage?: string, values?: any): string | undefined;
};

Yes, that is what I meant. Simplifying to this and just use it everywhere. Would mean some more existence checks in some places but I guess that could be good practice anyway as it would be "defense in depth" against bad behaving translators.

[sdirix]

LGTM Thanks

[lucas-koehler]

@sdirix I fixed the merge confligt in MIGRATION.md . Please re-approve :)