From 6f2fe1e7e97a18992c9aefe86d9f2242535f65cb Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 10 Nov 2023 10:21:10 -0800 Subject: [PATCH] Fix FBT whitespace handling (again (again)) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nice find, @mofeiZ! I really tried to thoroughly test all the examples I could think of for FBT whitespace but i missed the newline case. Initially Mofei found [this code potentially related to whitespace handling](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/fbt-nodes/FbtImplicitParamNode.js#L230-L233) but Babel never seems to produce consecutive JsxText nodes — this looks like maybe a leftover from older babel versions. I noticed that code wasn't actually trimming the whitspace but clearly it was happening somewhere, so i grepped for 'trim' and found that [this code](https://github.com/facebook/fbt/blob/0b4e0d13c30bffd0daa2a75715d606e3587b4e40/packages/babel-plugin-fbt/src/babel-processors/JSXFbtProcessor.js#L143) calls a [normalizeSpaces](https://github.com/facebook/fbt/blob/0b4e0d13c30bffd0daa2a75715d606e3587b4e40/packages/babel-plugin-fbt/src/FbtUtil.js#L86C10-L86C45) helper. Updating our logic to handle whitespace similarly just for children of fbt nodes produces the expected result. --- .../src/HIR/BuildHIR.ts | 30 ++++++++++++++++--- ...t.md => fbt-preserve-whitespace.expect.md} | 16 ++++------ ...espace.tsx => fbt-preserve-whitespace.tsx} | 4 +-- ...btparam-with-jsx-element-content.expect.md | 4 +-- .../packages/sprout/src/SproutTodoFilter.ts | 1 - 5 files changed, 35 insertions(+), 20 deletions(-) rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/{bug-fbt-preserve-whitespace.expect.md => fbt-preserve-whitespace.expect.md} (77%) rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/{bug-fbt-preserve-whitespace.tsx => fbt-preserve-whitespace.tsx} (82%) diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts index bdb285043c..3279de0041 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts @@ -1902,10 +1902,6 @@ function lowerExpression( const expr = exprPath as NodePath; const opening = expr.get("openingElement"); const tag = lowerJsxElementName(builder, opening.get("name")); - const children: Array = expr - .get("children") - .map((child) => lowerJsxElement(builder, child)) - .filter(notNull); const props: Array = []; for (const attribute of opening.get("attributes")) { if (attribute.isJSXSpreadAttribute()) { @@ -1982,6 +1978,32 @@ function lowerExpression( } props.push({ kind: "JsxAttribute", name: propName, place: value }); } + let children: Array; + if (tag.kind === "BuiltinTag" && tag.name === "fbt") { + children = expr + .get("children") + .map((child) => { + if (child.isJSXText()) { + /* + * FBT whitespace normalization differs from standard JSX: + * https://github.com/facebook/fbt/blob/0b4e0d13c30bffd0daa2a75715d606e3587b4e40/packages/babel-plugin-fbt/src/FbtUtil.js#L76-L87 + */ + const text = child.node.value.replace(/[^\S\u00A0]+/g, " "); + return lowerValueToTemporary(builder, { + kind: "JSXText", + value: text, + loc: exprLoc, + }); + } + return lowerJsxElement(builder, child); + }) + .filter(notNull); + } else { + children = expr + .get("children") + .map((child) => lowerJsxElement(builder, child)) + .filter(notNull); + } return { kind: "JsxExpression", tag, diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-fbt-preserve-whitespace.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbt-preserve-whitespace.expect.md similarity index 77% rename from compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-fbt-preserve-whitespace.expect.md rename to compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbt-preserve-whitespace.expect.md index f36e869e75..e770dad730 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-fbt-preserve-whitespace.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbt-preserve-whitespace.expect.md @@ -3,10 +3,8 @@ ```javascript import fbt from "fbt"; -/** - * TODO: remove this from SproutTodoFilter when fixed. - */ +const _ = fbt; function Component({ value }: { value: string }) { return ( @@ -28,17 +26,15 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; import fbt from "fbt"; -/** - * TODO: remove this from SproutTodoFilter when fixed. - */ -function Component(t11) { +const _ = fbt; +function Component(t12) { const $ = useMemoCache(2); - const { value } = t11; + const { value } = t12; let t0; if ($[0] !== value) { t0 = fbt._( - "Before text{paramName}", + "Before text {paramName}", [ fbt._param( "paramName", @@ -46,7 +42,7 @@ function Component(t11) { value ), ], - { hk: "3DIRxJ" } + { hk: "3z5SVE" } ); $[0] = value; $[1] = t0; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-fbt-preserve-whitespace.tsx b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbt-preserve-whitespace.tsx similarity index 82% rename from compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-fbt-preserve-whitespace.tsx rename to compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbt-preserve-whitespace.tsx index 79903b0462..a41d2e1a2f 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-fbt-preserve-whitespace.tsx +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbt-preserve-whitespace.tsx @@ -1,8 +1,6 @@ import fbt from "fbt"; -/** - * TODO: remove this from SproutTodoFilter when fixed. - */ +const _ = fbt; function Component({ value }: { value: string }) { return ( diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbtparam-with-jsx-element-content.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbtparam-with-jsx-element-content.expect.md index 83ffc40247..2b66b46a48 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbtparam-with-jsx-element-content.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/fbtparam-with-jsx-element-content.expect.md @@ -28,9 +28,9 @@ function Component({ name, data, icon }) { import { unstable_useMemoCache as useMemoCache } from "react"; import fbt from "fbt"; -function Component(t29) { +function Component(t33) { const $ = useMemoCache(4); - const { name, data, icon } = t29; + const { name, data, icon } = t33; let t0; if ($[0] !== name || $[1] !== icon || $[2] !== data) { t0 = ( diff --git a/compiler/packages/sprout/src/SproutTodoFilter.ts b/compiler/packages/sprout/src/SproutTodoFilter.ts index 8167c27ab1..d9acb43bf2 100644 --- a/compiler/packages/sprout/src/SproutTodoFilter.ts +++ b/compiler/packages/sprout/src/SproutTodoFilter.ts @@ -469,7 +469,6 @@ const skipFilter = new Set([ // Bug in Forget output "todo-rename-source-variables", - "bug-fbt-preserve-whitespace", // Tested e2e in forget-feedback repo "userspace-use-memo-cache",