fix transposed escape functions (#25534)

escapeTextForBrowser accepts any type so flow did not identify that we were escaping a Chunk rather than a string. It's tricky because we sometimes want to be able to escape non strings. I've also updated the types for `Chunk` and `escapeTextForBrowser` so that we should be able to catch this statically in the future. The reason this did not show up in tests is almost all of our tests of float (the areas affected by transpositions) are tested using the Node runtime where a chunk type is a string. It may be wise to run these tests in every runtime in the future or at least make sure there is broad representation of resources in each specific runtime test suite.
facebook · Oct 22, 2022 · 9341775 · 9341775
1 parent d1ced9f
commit 9341775
Show file tree

Hide file tree

Showing 7 changed files with 31 additions and 11 deletions.
diff --git a/packages/react-dom-bindings/src/server/ReactDOMLegacyServerStreamConfig.js b/packages/react-dom-bindings/src/server/ReactDOMLegacyServerStreamConfig.js
@@ -12,8 +12,8 @@ export interface Destination {
   destroy(error: Error): mixed;
 }
 
-export type PrecomputedChunk = string;
-export type Chunk = string;
+export opaque type PrecomputedChunk = string;
+export opaque type Chunk = string;
 
 export function scheduleWork(callback: () => void) {
   callback();

diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js
@@ -2385,7 +2385,7 @@ export function writeInitialResources(
     } else {
       target.push(
         precedencePlaceholderStart,
-        escapeTextForBrowser(stringToChunk(precedence)),
+        stringToChunk(escapeTextForBrowser(precedence)),
         precedencePlaceholderEnd,
       );
     }
@@ -2417,7 +2417,7 @@ export function writeInitialResources(
       case 'title': {
         pushStartTitleImpl(target, r.props, responseState);
         if (typeof r.props.children === 'string') {
-          target.push(escapeTextForBrowser(stringToChunk(r.props.children)));
+          target.push(stringToChunk(escapeTextForBrowser(r.props.children)));
         }
         pushEndInstance(target, target, 'title', r.props);
         break;
@@ -2518,7 +2518,7 @@ export function writeImmediateResources(
       case 'title': {
         pushStartTitleImpl(target, r.props, responseState);
         if (typeof r.props.children === 'string') {
-          target.push(escapeTextForBrowser(stringToChunk(r.props.children)));
+          target.push(stringToChunk(escapeTextForBrowser(r.props.children)));
         }
         pushEndInstance(target, target, 'title', r.props);
         break;

diff --git a/packages/react-dom-bindings/src/server/escapeTextForBrowser.js b/packages/react-dom-bindings/src/server/escapeTextForBrowser.js
@@ -28,6 +28,8 @@
  * CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
  * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
  * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * @flow
  */
 
 // code copied and modified from escape-html
@@ -103,12 +105,12 @@ function escapeHtml(string) {
  * @param {*} text Text value to escape.
  * @return {string} An escaped string.
  */
-function escapeTextForBrowser(text) {
+function escapeTextForBrowser(text: string | number | boolean): string {
   if (typeof text === 'boolean' || typeof text === 'number') {
     // this shortcircuit helps perf for types that we know will never have
     // special characters, especially given that this function is used often
     // for numeric dom ids.
-    return '' + text;
+    return '' + (text: any);
   }
   return escapeHtml(text);
 }

diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js
@@ -484,4 +484,22 @@ describe('ReactDOMFizzServerBrowser', () => {
 
     expect(errors).toEqual(['uh oh', 'uh oh']);
   });
+
+  // https://github.com/facebook/react/pull/25534/files - fix transposed escape functions
+  // @gate enableFloat
+  it('should encode title properly', async () => {
+    const stream = await ReactDOMFizzServer.renderToReadableStream(
+      <html>
+        <head>
+          <title>foo</title>
+        </head>
+        <body>bar</body>
+      </html>,
+    );
+
+    const result = await readResult(stream);
+    expect(result).toEqual(
+      '<!DOCTYPE html><html><head><title>foo</title></title></head><body>bar</body></html>',
+    );
+  });
 });
diff --git a/packages/react-server-dom-relay/src/ReactServerStreamConfigFB.js b/packages/react-server-dom-relay/src/ReactServerStreamConfigFB.js
@@ -14,8 +14,8 @@ export type Destination = {
   error: mixed,
 };
 
-export type PrecomputedChunk = string;
-export type Chunk = string;
+export opaque type PrecomputedChunk = string;
+export opaque type Chunk = string;
 
 export function scheduleWork(callback: () => void) {
   // We don't schedule work in this model, and instead expect performWork to always be called repeatedly.

diff --git a/packages/react-server/src/ReactServerStreamConfigBrowser.js b/packages/react-server/src/ReactServerStreamConfigBrowser.js
@@ -10,7 +10,7 @@
 export type Destination = ReadableStreamController;
 
 export type PrecomputedChunk = Uint8Array;
-export type Chunk = Uint8Array;
+export opaque type Chunk = Uint8Array;
 
 export function scheduleWork(callback: () => void) {
   callback();

diff --git a/packages/react-server/src/ReactServerStreamConfigNode.js b/packages/react-server/src/ReactServerStreamConfigNode.js
@@ -17,7 +17,7 @@ interface MightBeFlushable {
 export type Destination = Writable & MightBeFlushable;
 
 export type PrecomputedChunk = Uint8Array;
-export type Chunk = string;
+export opaque type Chunk = string;
 
 export function scheduleWork(callback: () => void) {
   setImmediate(callback);