Have ExtractedLocation contain null for locales that are not available (instead of undefined)

[Korri]

This will allow use to serialize and keep the information as our serializer removed undefined properties.

It will remove the necessity of a hack around it on web#10270

I decided to change only the ExtractedTranslations interface as it will minimise the impact on other systems, makes for a much simpler implementation (as undefined is the return value in case of failure of a lot of methods like Map.get), and its the only place we need null.

[nimzco]

Is this really needed?

[Korri]

Yes, this.translations is TranslationDictonary | undefined so null is not a valid value.

Do you think using

this.translations.set(id, translation === null ? undefined : translation);

instead be more readable?

[nimzco]

Oh right I missed that initialTranslations was an ExtractedTranslations. || undefined is fine with me 👍

[lemonmade]

I think I would rather change this so that this.translations has the same "no translation found" type as we give when you serialize it (null)

[nimzco]

The code looks good to me, thanks for this change and the test! 👍

[lemonmade]

Just one change I think would make things a bit simpler, then LGTM 👍

[lemonmade]

I think I would rather change this so that this.translations has the same "no translation found" type as we give when you serialize it (null)

[Korri]

Just one change I think would make things a bit simpler, then LGTM 👍

Hey @lemonmade, as I mentioned here:

I decided to change only the ExtractedTranslations interface as it will minimise the impact on other systems, makes for a much simpler implementation (as undefined is the return value in case of failure of a lot of methods like Map.get), and its the only place we need null.

I did do it that way originally, and it makes the code much more complicated (need to add a lot of || null everywhere, or some non-trivial things like changing the TranslationGetter interface to return null), so I decided against it, but if you still thinks its best I'm ok to do it.

[lemonmade]

I don't think any public API needs to change, just how we store them internally. You might have to update where we store them to do || null but I think it makes it conceptually simpler.