Null guard all interpolated strings passed to _t

[kegsay]

Comment should explain why.

[richvdh]

See comment at element-hq/element-web#4195 (comment): if the problem is about missing properties (as the description on that PR implies), this does nothing to fix it.

Is it our intention to keep this forever? It seems like a shame to have to do this check every single time we do a translation, and a better solution is not to pass in null/undefined in the first place.

[richvdh]

1. what's "" + here for? why would args[0] be truthy but not a string already?
2. why are you even bothering with this? why not just if (args[1] && typeof args[1] === 'object') ?

[kegsay]

• ""+ will stringify whatever args[0] is.
• args[0] could be anything given it's truthy (an object, an array, a boolean) and only string has indexOf so it'll crash if you try to call that on anything else.
• Annoyingly counterpart changes its behaviour depending on what args you pass it. The %( check is an attempt to detect when we're using it for interpolating strings, so we don't needlessly touch args[1].

[richvdh]

yeah, but do we ever actually use any of those alternative behaviours? Feels to me like the check for %( is costing more than it buys.

[kegsay]

I don't know if we use those alternative behaviours, as we call this function in a bazillion places so I can't reasonably check them.

[richvdh]

yeah, but what's the worst case? I can't imagine a case where if (args[1] && typeof args[1] === 'object') will be true, and replacing the null/undef members will be harmful.

[kegsay]

Is it our intention to keep this forever?

Yes.

It seems like a shame to have to do this check every single time we do a translation

It needs to be done either here or further up the stack in the actual code. Performance is hardly going to be an issue here.

a better solution is not to pass in null/undefined in the first place

If you want to add null guards to all interpolated values knock yourself out, I think it's a waste of time. null or undefined values are completely valid in ES6 template strings so we've never had a reason to check for them in the code previously. As a result, it's likely that there will be fun edge cases where they can be undefined resulting in the app crashing. Alternative suggestions welcome.

[richvdh]

Performance is hardly going to be an issue here.

uhhh... debatable.

My only other suggestion (which apparently I forgot to write) would be to warn when we're making this replacement, so that we can try to fix them further up the stack. I agree it's marginal though.

Also: tests have failed, though I suspect it's not your fault.

[kegsay]

warn when we're making this replacement

Done. Is there anything else for me to do here? Are we keeping the %( check or removing it?

[richvdh]

Are we keeping the %( check or removing it?

I vote for removing it. Up to you at the end of the day, though.

LGTM otherwise.

[kegsay]

Removed the check.