Multiple bugs in substituteTokens

cpojer commented 5 months ago

🐛 Bug Report

This block of code has two bugs:

invariant in a __DEV__ block which changes execution flow depending on whether fbt is running in the dev or prod environment. This violates Facebook's engineering principles from 2011-2021. RIP.
In production, missing params will be replaced with undefined in strings because of the argument === null check. This check should be argument == null instead.

To Reproduce

Create an fbt string like fbt('Test string with {missing} substitution'). In dev, this code will throw an error. In prod, it will replace {missing} with "undefined".

Expected behavior

Either throw both in dev and prod if there is a param mismatch, or throw in neither places.
Do not substitute missing params with undefined.

kayhadrin commented 5 months ago

Thanks for submitting this bug report.

I think we indeed should also use invariant() in prod too, but I'm not in a position to test this anymore since leaving the company. So for simplicity, this error scenario should be probably just logged. I don't remember why argument being null or undefined are treated differently right now though. Personally, I'd rather see undefined resolved as the string "undefined" rather than an empty string because it makes the issue easier to spot.

For the bt() call with a missing param, this error case should have been detected during the string extraction phase I reckon; but it's good that the runtime js code still verifies that arguments are provided properly.

Can I ask in which circumstances you detected this issue?

Regards,

David

On Tue, Apr 9, 2024, 14:41 Christoph Nakazawa @.***> wrote:

🐛 Bug Report

This block of code https://github.com/facebook/fbt/blob/dd54f9ecb78c7a72ed9e9c42867ab3c2da08852f/runtime/shared/substituteTokens.js#L100-L120 has two bugs:

invariant in a DEV block which changes execution flow depending on whether fbt is running in the dev or prod environment. This violates Facebook's engineering principles from 2011-2021. RIP.

In production, missing params will be replaced with undefined in strings because of the argument === null check. This check should be argument == null instead.

To Reproduce

Create an fbt string like fbt('Test string with {missing} substitution'). In dev, this code will throw an error. In prod, it will replace {missing} with "undefined".

Expected behavior

Either throw both in dev and prod if there is a param mismatch, or throw in neither places.

Do not substitute missing params with undefined.

— Reply to this email directly, view it on GitHub https://github.com/facebook/fbt/issues/412, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABMS5EBC7LA4OZ3D5NATFDY4N5QXAVCNFSM6AAAAABF54ZN5WVHI2DSMVQWIX3LMV43ASLTON2WKOZSGIZTENRUGIZDOMY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

cpojer commented 5 months ago

Admittedly, I have a funky use-case here:

I have user-generated input that is extracted from the db and translated.
Users may change the initial text which may or may not provide the same params as before.
I have custom params that end up using the same token delimiters ({ and }) because otherwise it doesn't work well with my translation provider (crowdin). Those params are substituted from the translated string with user-generated data separately from fbt.

With all of these constraints combined, I had a try-catch around my fbt calls for user-generated text, since the invariant would crash the game rather than falling back to the non-translated input. I then realized that the invariant was skipped and undefined was inserted. I worked around by patching fbt, which works great for me.

Here is a real scenario I can imagine happening many times at Facebook:

Dev amends code before landing without trying it. Reviewer approves without trying it.
Code is actually wrong, but doesn't throw in prod because the invariant is dev only.
Code now renders "undefined" in place of the param in a user-facing string in prod.

cpojer commented 4 months ago

Fixed by @voideanvalue in https://github.com/facebook/fbt/commit/bd915824e686043af881248bcc62b41e2a595893 and https://github.com/facebook/fbt/commit/f409115582f1fc7afad4f2ca776215aedd80e2ff.

kayhadrin commented 4 months ago

@voideanvalue Awesome, thanks for the fix!

On Wed, Apr 17, 2024, 10:08 Christoph Nakazawa @.***> wrote:

Closed #412 https://github.com/facebook/fbt/issues/412 as completed.

— Reply to this email directly, view it on GitHub https://github.com/facebook/fbt/issues/412#event-12498991039, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABMS5BJ3EDMJS5MZDXQTS3Y5XDSFAVCNFSM6AAAAABF54ZN5WVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGQ4TQOJZGEYDGOI . You are receiving this because you commented.Message ID: @.***>

facebook / fbt