Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fizz] Send errors down to client #24551

Merged
merged 7 commits into from May 30, 2022

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented May 12, 2022

This is a continuation of @salazarm's PR #24202

Errors are now modeled as

  1. hash (prod and dev)
  2. message (dev only)
  3. component stack (dev onlhy)

They are sent in html if already known when flushing (e.g. flushing boundary in fallback)
They are also sent in a script in the client render instruction (e.g. a pending boundary rejects and needs to render on client)

Additionally all error content is escaped

@sizebot
Copy link

sizebot commented May 12, 2022

Comparing: a276638...c026c26

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.20% 131.28 kB 131.54 kB +0.30% 42.13 kB 42.26 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.19% 136.54 kB 136.80 kB +0.30% 43.68 kB 43.81 kB
facebook-www/ReactDOM-prod.classic.js +0.11% 439.35 kB 439.85 kB +0.19% 80.29 kB 80.44 kB
facebook-www/ReactDOM-prod.modern.js +0.12% 424.64 kB 425.14 kB +0.19% 78.13 kB 78.29 kB
facebook-www/ReactDOMForked-prod.classic.js +0.11% 439.35 kB 439.85 kB +0.19% 80.29 kB 80.44 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.production.min.js +3.63% 34.09 kB 35.33 kB +4.82% 11.59 kB 12.15 kB
oss-stable/react-dom/cjs/react-dom-server.browser.production.min.js +3.62% 34.11 kB 35.35 kB +4.80% 11.61 kB 12.17 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.min.js +3.58% 34.53 kB 35.77 kB +4.74% 11.76 kB 12.32 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.production.min.js +3.54% 34.22 kB 35.43 kB +4.57% 11.72 kB 12.25 kB
oss-stable/react-dom/umd/react-dom-server.browser.production.min.js +3.54% 34.24 kB 35.45 kB +4.55% 11.74 kB 12.27 kB
oss-experimental/react-dom/umd/react-dom-server.browser.production.min.js +3.49% 34.66 kB 35.87 kB +4.74% 11.87 kB 12.43 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.production.min.js +3.37% 37.36 kB 38.62 kB +4.08% 12.66 kB 13.18 kB
oss-stable/react-dom/cjs/react-dom-server.node.production.min.js +3.37% 37.38 kB 38.64 kB +4.07% 12.68 kB 13.20 kB
facebook-www/ReactDOMServerStreaming-prod.modern.js +3.36% 79.24 kB 81.90 kB +3.84% 16.78 kB 17.43 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js +3.32% 37.86 kB 39.12 kB +4.08% 12.84 kB 13.36 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +3.28% 32.96 kB 34.04 kB +4.42% 10.84 kB 11.32 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +3.28% 32.98 kB 34.06 kB +4.40% 10.86 kB 11.34 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +3.23% 33.40 kB 34.48 kB +4.40% 11.01 kB 11.49 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.production.min.js +3.16% 33.11 kB 34.15 kB +4.16% 11.00 kB 11.45 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.production.min.js +3.16% 33.13 kB 34.18 kB +4.14% 11.02 kB 11.47 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.production.min.js +3.11% 33.55 kB 34.59 kB +4.04% 11.16 kB 11.61 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.production.min.js +3.05% 36.65 kB 37.76 kB +3.65% 12.08 kB 12.52 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.production.min.js +3.05% 36.67 kB 37.79 kB +3.64% 12.10 kB 12.54 kB
facebook-www/ReactDOMServer-prod.modern.js +3.03% 75.71 kB 78.01 kB +3.94% 15.74 kB 16.36 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.production.min.js +3.01% 37.15 kB 38.26 kB +3.60% 12.26 kB 12.70 kB
facebook-www/ReactDOMServer-dev.modern.js +2.96% 238.42 kB 245.47 kB +2.87% 57.05 kB 58.69 kB
facebook-www/ReactDOMServerStreaming-dev.modern.js +2.94% 234.94 kB 241.84 kB +2.84% 56.26 kB 57.86 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.development.js +2.88% 233.23 kB 239.96 kB +2.74% 56.66 kB 58.21 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.development.js +2.88% 233.26 kB 239.98 kB +2.74% 56.69 kB 58.24 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.development.js +2.86% 234.80 kB 241.53 kB +2.68% 57.11 kB 58.64 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.development.js +2.86% 234.94 kB 241.67 kB +2.74% 57.13 kB 58.69 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.development.js +2.86% 234.97 kB 241.69 kB +2.74% 57.16 kB 58.72 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.development.js +2.85% 244.75 kB 251.74 kB +2.71% 57.31 kB 58.86 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.development.js +2.85% 244.78 kB 251.76 kB +2.71% 57.34 kB 58.89 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.development.js +2.84% 236.51 kB 243.24 kB +2.74% 57.58 kB 59.16 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.development.js +2.84% 246.41 kB 253.40 kB +2.75% 57.71 kB 59.30 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.development.js +2.83% 232.83 kB 239.42 kB +2.62% 56.93 kB 58.42 kB
oss-stable/react-dom/cjs/react-dom-server.browser.development.js +2.83% 232.85 kB 239.44 kB +2.62% 56.95 kB 58.44 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.development.js +2.81% 233.99 kB 240.58 kB +2.62% 56.86 kB 58.35 kB
oss-stable/react-dom/cjs/react-dom-server.node.development.js +2.81% 234.02 kB 240.60 kB +2.62% 56.89 kB 58.38 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js +2.81% 234.40 kB 240.98 kB +2.66% 57.33 kB 58.86 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.development.js +2.80% 244.32 kB 251.16 kB +2.67% 57.54 kB 59.08 kB
oss-stable/react-dom/umd/react-dom-server.browser.development.js +2.80% 244.34 kB 251.18 kB +2.68% 57.56 kB 59.11 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js +2.80% 235.56 kB 242.15 kB +2.64% 57.27 kB 58.78 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js +2.78% 245.98 kB 252.82 kB +2.67% 57.94 kB 59.49 kB
oss-stable-semver/react-server/cjs/react-server.development.js +2.57% 126.28 kB 129.53 kB +2.46% 31.63 kB 32.41 kB
oss-stable/react-server/cjs/react-server.development.js +2.57% 126.28 kB 129.53 kB +2.46% 31.63 kB 32.41 kB
oss-experimental/react-server/cjs/react-server.development.js +2.55% 127.15 kB 130.40 kB +2.46% 31.83 kB 32.62 kB
oss-stable-semver/react-server/cjs/react-server.production.min.js +2.54% 20.36 kB 20.87 kB +2.61% 6.97 kB 7.15 kB
oss-stable/react-server/cjs/react-server.production.min.js +2.54% 20.36 kB 20.87 kB +2.61% 6.97 kB 7.15 kB
oss-experimental/react-server/cjs/react-server.production.min.js +2.49% 20.69 kB 21.20 kB +2.62% 7.07 kB 7.25 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-dom/cjs/react-dom-server.browser.production.min.js +3.63% 34.09 kB 35.33 kB +4.82% 11.59 kB 12.15 kB
oss-stable/react-dom/cjs/react-dom-server.browser.production.min.js +3.62% 34.11 kB 35.35 kB +4.80% 11.61 kB 12.17 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.min.js +3.58% 34.53 kB 35.77 kB +4.74% 11.76 kB 12.32 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.production.min.js +3.54% 34.22 kB 35.43 kB +4.57% 11.72 kB 12.25 kB
oss-stable/react-dom/umd/react-dom-server.browser.production.min.js +3.54% 34.24 kB 35.45 kB +4.55% 11.74 kB 12.27 kB
oss-experimental/react-dom/umd/react-dom-server.browser.production.min.js +3.49% 34.66 kB 35.87 kB +4.74% 11.87 kB 12.43 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.production.min.js +3.37% 37.36 kB 38.62 kB +4.08% 12.66 kB 13.18 kB
oss-stable/react-dom/cjs/react-dom-server.node.production.min.js +3.37% 37.38 kB 38.64 kB +4.07% 12.68 kB 13.20 kB
facebook-www/ReactDOMServerStreaming-prod.modern.js +3.36% 79.24 kB 81.90 kB +3.84% 16.78 kB 17.43 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js +3.32% 37.86 kB 39.12 kB +4.08% 12.84 kB 13.36 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +3.28% 32.96 kB 34.04 kB +4.42% 10.84 kB 11.32 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +3.28% 32.98 kB 34.06 kB +4.40% 10.86 kB 11.34 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +3.23% 33.40 kB 34.48 kB +4.40% 11.01 kB 11.49 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.production.min.js +3.16% 33.11 kB 34.15 kB +4.16% 11.00 kB 11.45 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.production.min.js +3.16% 33.13 kB 34.18 kB +4.14% 11.02 kB 11.47 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.production.min.js +3.11% 33.55 kB 34.59 kB +4.04% 11.16 kB 11.61 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.production.min.js +3.05% 36.65 kB 37.76 kB +3.65% 12.08 kB 12.52 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.production.min.js +3.05% 36.67 kB 37.79 kB +3.64% 12.10 kB 12.54 kB
facebook-www/ReactDOMServer-prod.modern.js +3.03% 75.71 kB 78.01 kB +3.94% 15.74 kB 16.36 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.production.min.js +3.01% 37.15 kB 38.26 kB +3.60% 12.26 kB 12.70 kB
facebook-www/ReactDOMServer-dev.modern.js +2.96% 238.42 kB 245.47 kB +2.87% 57.05 kB 58.69 kB
facebook-www/ReactDOMServerStreaming-dev.modern.js +2.94% 234.94 kB 241.84 kB +2.84% 56.26 kB 57.86 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.development.js +2.88% 233.23 kB 239.96 kB +2.74% 56.66 kB 58.21 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.development.js +2.88% 233.26 kB 239.98 kB +2.74% 56.69 kB 58.24 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.development.js +2.86% 234.80 kB 241.53 kB +2.68% 57.11 kB 58.64 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.development.js +2.86% 234.94 kB 241.67 kB +2.74% 57.13 kB 58.69 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.development.js +2.86% 234.97 kB 241.69 kB +2.74% 57.16 kB 58.72 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.development.js +2.85% 244.75 kB 251.74 kB +2.71% 57.31 kB 58.86 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.development.js +2.85% 244.78 kB 251.76 kB +2.71% 57.34 kB 58.89 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.development.js +2.84% 236.51 kB 243.24 kB +2.74% 57.58 kB 59.16 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.development.js +2.84% 246.41 kB 253.40 kB +2.75% 57.71 kB 59.30 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.development.js +2.83% 232.83 kB 239.42 kB +2.62% 56.93 kB 58.42 kB
oss-stable/react-dom/cjs/react-dom-server.browser.development.js +2.83% 232.85 kB 239.44 kB +2.62% 56.95 kB 58.44 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.development.js +2.81% 233.99 kB 240.58 kB +2.62% 56.86 kB 58.35 kB
oss-stable/react-dom/cjs/react-dom-server.node.development.js +2.81% 234.02 kB 240.60 kB +2.62% 56.89 kB 58.38 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js +2.81% 234.40 kB 240.98 kB +2.66% 57.33 kB 58.86 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.development.js +2.80% 244.32 kB 251.16 kB +2.67% 57.54 kB 59.08 kB
oss-stable/react-dom/umd/react-dom-server.browser.development.js +2.80% 244.34 kB 251.18 kB +2.68% 57.56 kB 59.11 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js +2.80% 235.56 kB 242.15 kB +2.64% 57.27 kB 58.78 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js +2.78% 245.98 kB 252.82 kB +2.67% 57.94 kB 59.49 kB
oss-stable-semver/react-server/cjs/react-server.development.js +2.57% 126.28 kB 129.53 kB +2.46% 31.63 kB 32.41 kB
oss-stable/react-server/cjs/react-server.development.js +2.57% 126.28 kB 129.53 kB +2.46% 31.63 kB 32.41 kB
oss-experimental/react-server/cjs/react-server.development.js +2.55% 127.15 kB 130.40 kB +2.46% 31.83 kB 32.62 kB
oss-stable-semver/react-server/cjs/react-server.production.min.js +2.54% 20.36 kB 20.87 kB +2.61% 6.97 kB 7.15 kB
oss-stable/react-server/cjs/react-server.production.min.js +2.54% 20.36 kB 20.87 kB +2.61% 6.97 kB 7.15 kB
oss-experimental/react-server/cjs/react-server.production.min.js +2.49% 20.69 kB 21.20 kB +2.62% 7.07 kB 7.25 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.browser.development.server.js +1.38% 61.16 kB 62.00 kB +1.39% 15.85 kB 16.08 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.browser.development.server.js +1.38% 61.16 kB 62.00 kB +1.39% 15.85 kB 16.08 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.browser.development.server.js +1.37% 61.22 kB 62.06 kB +1.35% 15.88 kB 16.09 kB
oss-stable-semver/react-server-dom-webpack/umd/react-server-dom-webpack-writer.browser.development.server.js +1.34% 64.32 kB 65.18 kB +1.29% 16.08 kB 16.29 kB
oss-stable/react-server-dom-webpack/umd/react-server-dom-webpack-writer.browser.development.server.js +1.34% 64.32 kB 65.18 kB +1.29% 16.08 kB 16.29 kB
oss-experimental/react-server-dom-webpack/umd/react-server-dom-webpack-writer.browser.development.server.js +1.34% 64.38 kB 65.24 kB +1.27% 16.10 kB 16.31 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.node.development.server.js +1.34% 62.94 kB 63.78 kB +1.38% 16.05 kB 16.27 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.node.development.server.js +1.34% 62.94 kB 63.78 kB +1.38% 16.05 kB 16.27 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.node.development.server.js +1.34% 62.99 kB 63.83 kB +1.33% 16.07 kB 16.29 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.browser.production.min.server.js +1.03% 15.89 kB 16.05 kB +1.13% 5.95 kB 6.01 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.browser.production.min.server.js +1.03% 15.89 kB 16.05 kB +1.13% 5.95 kB 6.01 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.browser.production.min.server.js +1.02% 15.93 kB 16.10 kB +1.12% 5.97 kB 6.04 kB
oss-stable-semver/react-server-dom-webpack/umd/react-server-dom-webpack-writer.browser.production.min.server.js +1.02% 16.08 kB 16.25 kB +1.10% 6.00 kB 6.06 kB
oss-stable/react-server-dom-webpack/umd/react-server-dom-webpack-writer.browser.production.min.server.js +1.02% 16.08 kB 16.25 kB +1.10% 6.00 kB 6.06 kB
oss-experimental/react-server-dom-webpack/umd/react-server-dom-webpack-writer.browser.production.min.server.js +1.02% 16.12 kB 16.29 kB +1.08% 6.02 kB 6.08 kB
oss-stable-semver/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.node.production.min.server.js +0.99% 16.47 kB 16.64 kB +1.13% 6.10 kB 6.17 kB
oss-stable/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.node.production.min.server.js +0.99% 16.47 kB 16.64 kB +1.13% 6.10 kB 6.17 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-writer.node.production.min.server.js +0.99% 16.52 kB 16.68 kB +1.14% 6.13 kB 6.20 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.min.js +0.25% 92.43 kB 92.66 kB +0.36% 28.48 kB 28.58 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.min.js +0.25% 92.45 kB 92.68 kB +0.36% 28.50 kB 28.60 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js +0.24% 97.30 kB 97.53 kB +0.30% 29.90 kB 29.99 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.profiling.min.js +0.23% 101.25 kB 101.48 kB +0.37% 30.75 kB 30.87 kB
oss-stable/react-reconciler/cjs/react-reconciler.profiling.min.js +0.23% 101.27 kB 101.50 kB +0.38% 30.77 kB 30.89 kB
oss-experimental/react-reconciler/cjs/react-reconciler.profiling.min.js +0.22% 106.12 kB 106.35 kB +0.31% 32.14 kB 32.24 kB
oss-stable-semver/react-art/cjs/react-art.production.min.js +0.21% 81.52 kB 81.69 kB +0.35% 25.38 kB 25.47 kB
oss-stable/react-art/cjs/react-art.production.min.js +0.21% 81.55 kB 81.72 kB +0.35% 25.38 kB 25.47 kB
oss-stable-semver/react-dom/umd/react-dom.production.min.js +0.20% 131.41 kB 131.67 kB +0.29% 42.79 kB 42.92 kB
oss-stable/react-dom/umd/react-dom.production.min.js +0.20% 131.43 kB 131.70 kB +0.29% 42.79 kB 42.91 kB
oss-stable-semver/react-dom/cjs/react-dom.production.min.js +0.20% 131.25 kB 131.52 kB +0.30% 42.13 kB 42.26 kB
oss-stable/react-dom/cjs/react-dom.production.min.js +0.20% 131.28 kB 131.54 kB +0.30% 42.13 kB 42.26 kB

Generated by 🚫 dangerJS against c026c26

@gnoff gnoff requested a review from sebmarkbage May 12, 2022
if (errorComponentStack === null) {
errorComponentStack =
'[A component stack was not captured at the point of origin of the original error. ' +
'This Component stack is from the boundary handling the error.]\n' +
Copy link
Collaborator

@sebmarkbage sebmarkbage May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component stacks are parsed. The format is the same as the native stacks also any error parser can do it. This introduces a new part to that string that can break parsers.

It’s not only human readable.

Copy link
Collaborator Author

@gnoff gnoff May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... good to know. i'll remove. I suppose it's not that useful and should be impossible today

return writeChunkAndReturn(destination, clientRenderScript2);
}

const regexForJSStringsInScripts = /[<\'\"\`\n\u2028\u2029]/g;
Copy link
Collaborator

@sebmarkbage sebmarkbage May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this enough? JSON.stringify does more.

It might be worth doing JSON.stringify first and then escape the extras.

Copy link
Collaborator Author

@gnoff gnoff May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could but that would create a string wrapped in extra \" quotes which I'd have to strip out. any downside to adding the missing characters JSON.stringify escapes?

Copy link
Collaborator Author

@gnoff gnoff May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decided to go the stringify route

@gnoff gnoff requested a review from sebmarkbage May 13, 2022
// santizing breaking out of strings and script tags
case '<':
return '\\u003c';
case "'":
Copy link
Collaborator

@sebmarkbage sebmarkbage May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Copy link
Collaborator Author

@gnoff gnoff May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strictly speaking it isn't since we control the wrapping quotations. but if we did a careless quote replacement for our strings and ended up with single quotes for attributes then this would allow escaping. I put it here so that even if we change later the quotes used we won't have to remember to modify the escape procedure

Copy link
Collaborator Author

@gnoff gnoff May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I'm wrong. I think I was mixing up the html attribute context with the js argument one when i was deciding to include these


const regexForJSStringsInScripts = /[<\'\`\u2028\u2029]/g;
function escapeJSStringsForInstructionScripts(input: string): string {
if (typeof input !== 'string') {
Copy link
Collaborator

@sebmarkbage sebmarkbage May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is against an internal value that's already type checked. You can exclude this here.

It would be better to check in inside ReactFizzServer when it first comes from user space code (e.g. check the return value of onRecoverableError) so that the internal type that travels around inside Fizz is sound. It also means that the error can be more specific and we don't have to replicate it in other format configs.

We might do type checks against things like props in here that are unknown before and specific to this host config but this source of the error values aren't specific.

Copy link
Collaborator Author

@gnoff gnoff May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(e.g. check the return value of onRecoverableError)

do you mean onError?

It also means that the error can be more specific and we don't have to replicate it in other format configs.

I don't understand what you mean here.

It would be better to check in inside ReactFizzServer when it first comes from user space code

yeah this makes sense to me

});
if (escaped[0] === '"' && escaped[escaped.length - 1] === '"') {
// This should always be true since JSON.stringify of a string produces a value wrapped in double quotes.
return escaped.slice(1, -1);
Copy link
Collaborator

@sebmarkbage sebmarkbage May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of slicing, why don't you just remove these from the clientRenderErrorScriptArgInterstitial etc?

Copy link
Collaborator Author

@gnoff gnoff May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, makes sense

),
serverError
? // eslint-disable-next-line react-internal/prod-error-codes
new Error(serverError)
Copy link
Collaborator

@sebmarkbage sebmarkbage May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're going to do a follow up PR to expand the onRecoverableError API, right? Like add a componentStack?

In that API where does the hash go?

I could imagine for example that the hash goes into the errorInfo object as a second argument:

onRecoverableError(error, {
  componentStack: '...',
  hash: '...',
});

In that world the error message would just be message. So I think we just make this new Error(errorMessage) for parity with other errors.

The hash could also alternatively go onto this error object itself because it's not applicable to other errors passed to onRecoverableError so the argument on errorInfo isn't necessarily generally applicable.

let error = new Error(errorMessage);
error.hash = errorHash;

Copy link
Collaborator Author

@gnoff gnoff May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default error reports (reportError and console.error) don't expose ad hoc properties in the console so attaching them to the error object will not actually expose them unless one implements onRecoverableError and knows to look for them

if we pass a second arg to reportError it is ignored, but if we pass a second arg to console.error (through the defaultOnRecoverableError implementation) we would get a visible rendition of the errorInfo but we would need to stop using reportError as the default handler.

Do you expect frameworks to implement a baseline defualt onRecoverableError handler that knows how to unwrap and utilize these extra properties so that the lack of default exposure is not a concern?

As for using the errorMessage, are you suggesting we use the "there was an error on the server, switched to client rendering" message that we fall back to if no message is provided (in Dev) in prod 100% of the time?

Copy link
Collaborator

@sebmarkbage sebmarkbage May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you expect frameworks to implement a baseline defualt onRecoverableError handler that knows how to unwrap and utilize these extra properties so that the lack of default exposure is not a concern?

Yes in particular for the hash. For one you wouldn't have a hash in the first place unless you also implemented onError on the server and the hash is useless to someone that doesn't have a storage for the value that they can look up. You'd have to know to do this. I expect a richer environment to just do that automatically translate the hash to a real error message in the UI for prod.

As for using the errorMessage, are you suggesting we use the "there was an error on the server, switched to client rendering" message that we fall back to if no message is provided (in Dev) in prod 100% of the time?

Yea and in DEV it would be the original message on the server.

Perhaps as a convenience we can also include the hash only in the prod error message similar to what we do with the minified error message.

Copy link
Collaborator Author

@gnoff gnoff May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps as a convenience we can also include the hash only in the prod error message similar to what we do with the minified error message.

ah like

`The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering. (Server Error Hash: ${theHash})`

Copy link
Collaborator

@sebmarkbage sebmarkbage May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea. I didn’t want to mess with the error message in dev since it’s user provided but for our own it’s fine.

@gnoff gnoff force-pushed the recoverableErrorServerToClient branch 2 times, most recently from 8ba9d5e to f88fff1 Compare May 18, 2022
}
return {errorMessage, errorComponentStack, errorHash};
}
export function getSuspenseInstanceFallbackErrorHash(
Copy link
Collaborator

@sebmarkbage sebmarkbage May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is no longer used. Should it be used in DEV? The only reason the other one should be used in prod is because it might contain an error message but only for the one case that maybe should be a hash anyway.

Copy link
Collaborator Author

@gnoff gnoff May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I actually removed it but github is showing it in the diff for some reason... have you see that before? Makes it hard to trust diffs...

@@ -1409,6 +1499,8 @@ function abortTask(task: Task): void {

if (!boundary.forceClientRender) {
boundary.forceClientRender = true;
boundary.errorMessage =
'This Suspense boundary was aborted by the server';
Copy link
Collaborator

@sebmarkbage sebmarkbage May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For renderToString we send this for every suspense boundary since that works by aborting early.

This seems a little long for production behavior, especially if we've already timed out we might want to keep it short.

I guess the reason we do this is because we don't to treat this as an error and call onError so we don't have a hash for it. Should we treat this as an error though? Maybe you want to know about what parts got aborted.

We could also maybe just assume on the client that if we don't have any hash (like undefined/missing attribute as opposed to empty string) that it was aborted on the server?

Copy link
Collaborator Author

@gnoff gnoff May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assume on the client that if we don't have any hash ... that it was aborted on the server?

hmm, if you don't use onError on the server the hash is going to be null (currently) and that doesn't get emitted on the template or render instruction. We'd have to distinguish between does not exist (we didn't call onError) and exists but empty (user did not set up onError) and would lead to extra attributes (data-hash) on every non-abort error. This feels fragile too because it would fall apart the moment we had a different kind of error case not requiring a user onError.

maybe there is a different kind of thing like data-sig(nal) that uses an enum for things like abort? it'd make the instruction scripts a bit longer but that's amortized over each client rendered boundary

}
if (__DEV__) {
if (errorComponentStack) {
(error: any).componentStack = errorComponentStack;
Copy link
Collaborator

@sebmarkbage sebmarkbage May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add the componentStack this way since we already have a separate protocol for that which is errorInfo.

In this first PR you can just exclude it.

Now that I think about it, it probably makes the most sense to do the same with the hash.

Regardless, let's not expose anything extra to users until we're done with the API.

Copy link
Collaborator Author

@gnoff gnoff May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roger

@@ -1132,9 +1176,37 @@ function validateIterable(iterable, iteratorFn: Function): void {
}
}

let renderNodeDestructive = renderNodeDestructiveImpl;
Copy link
Collaborator

@sebmarkbage sebmarkbage May 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use mutations in module scope. This messes with the compiler optimizations and resolving of cyclic dependencies because now things start getting dependent on ordering.

Instead create a function that inside of it does the DEV check. It'll get inlined.

// Currently if a lazy element rejects the component stack will include this
// special frame. My current intuition is this is good and will aid in identifying
// where errors are happening.
pushBuiltInComponentStackInDEV(task, '~lazy-element~');
Copy link
Collaborator

@sebmarkbage sebmarkbage May 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Fiber calls this 'Lazy'.

case LazyComponent:
return describeBuiltInComponentFrame('Lazy', source, owner);

This Fiber only exists temporarily before we know what the component will be and then it changes tag.

Which is similar to this case.

So I think it makes sense to keep it but call it 'Lazy' instead for parity.

Copy link
Collaborator Author

@gnoff gnoff May 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah maybe I was being pedantic in thinking that distinguishing between Component and elements that are lazy would matter but in reflection it probably would make more sense to the average engineer that any React.Lazy shows up as a Lazy in the stack even if technically it's not in Component position

Copy link
Collaborator

@sebmarkbage sebmarkbage May 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, then I misunderstood. I thought this was in the type position.

In that case having no stack frame would align more with the client.

It might be more confusing that you end up with two different errors for the client-side renders and the server-side renders. Like deduping techniques would get confused.

Copy link
Collaborator Author

@gnoff gnoff May 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that make sense

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Two nits above but other than that looks good!

@gnoff gnoff force-pushed the recoverableErrorServerToClient branch 2 times, most recently from 110a01f to e34e1df Compare May 30, 2022
salazarm and others added 6 commits May 30, 2022
This commit adds another way to get errors to the suspense instance by encoding them as dataset properties of a template element at the head of the boundary. Previously if there was an error before the boundary flushed there was no way to stream the error to the client because there would never be a client render instruction.

Additionally the error is sent in 3 parts

1) error hash - this is always sent (dev or prod) if one is provided
2) error message - Dev only
3) error component stack - Dev only, this now captures the stack at the point of error

Another item addressed in this commit is the escaping of potentially unsafe data. all error components are escaped as test for browers when written into the html and as javascript strings when written into a client render instruction.
@gnoff gnoff force-pushed the recoverableErrorServerToClient branch from e34e1df to 3fc4439 Compare May 30, 2022
@gnoff gnoff merged commit aec5759 into facebook:main May 30, 2022
36 checks passed
@gnoff gnoff deleted the recoverableErrorServerToClient branch May 30, 2022
@mylesgodino mylesgodino mentioned this pull request Jun 4, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jun 6, 2022
Summary:
This sync includes the following changes:
- **[dd4950c90](facebook/react@dd4950c90 )**: [Flight] Implement useId hook ([#24172](facebook/react#24172)) //<Josh Story>//
- **[26a5b3c7f](facebook/react@26a5b3c7f )**: Explicitly set `highWaterMark` to 0 for `ReadableStream` ([#24641](facebook/react#24641)) //<Josh Larson>//
- **[aec575914](facebook/react@aec575914 )**: [Fizz] Send errors down to client ([#24551](facebook/react#24551)) //<Josh Story>//
- **[a2766387e](facebook/react@a2766387e )**: [Fizz] Improve text separator byte efficiency ([#24630](facebook/react#24630)) //<Josh Story>//
- **[f7860538a](facebook/react@f7860538a )**: Fix typo in useSyncExternalStore main entry point error ([#24631](facebook/react#24631)) //<François Chalifour>//
- **[1bed20731](facebook/react@1bed20731 )**: Add a module map option to the Webpack Flight Client ([#24629](facebook/react#24629)) //<Sebastian Markbåge>//
- **[b2763d3ea](facebook/react@b2763d3ea )**: Move hydration code out of normal Suspense path ([#24532](facebook/react#24532)) //<Andrew Clark>//
- **[357a61324](facebook/react@357a61324 )**: [DevTools][Transition Tracing] Added support for Suspense Boundaries ([#23365](facebook/react#23365)) //<Luna Ruan>//
- **[2c8a1452b](facebook/react@2c8a1452b )**: Fix ignored setState in Safari when iframe is touched ([#24459](facebook/react#24459)) //<dan>//
- **[62662633d](facebook/react@62662633d )**: Remove enableFlipOffscreenUnhideOrder ([#24545](facebook/react#24545)) //<Ricky>//
- **[34da5aa69](facebook/react@34da5aa69 )**: Only treat updates to lazy as a new mount in legacy mode ([#24530](facebook/react#24530)) //<Ricky>//
- **[46a6d77e3](facebook/react@46a6d77e3 )**: Unify JSResourceReference Interfaces ([#24507](facebook/react#24507)) //<Timothy Yung>//
- **[6cbf0f7fa](facebook/react@6cbf0f7fa )**: Fork ReactSymbols ([#24484](facebook/react#24484)) //<Ricky>//
- **[a10a9a6b5](facebook/react@a10a9a6b5 )**: Add test for hiding children after layout destroy ([#24483](facebook/react#24483)) //<Ricky>//
- **[b4eb0ad71](facebook/react@b4eb0ad71 )**: Do not replay erroring beginWork with invokeGuardedCallback when suspended or previously errored ([#24480](facebook/react#24480)) //<Josh Story>//
- **[99eef9e2d](facebook/react@99eef9e2d )**: Hide children of Offscreen after destroy effects ([#24446](facebook/react#24446)) //<Ricky>//
- **[ce1386028](facebook/react@ce1386028 )**: Remove enablePersistentOffscreenHostContainer flag ([#24460](facebook/react#24460)) //<Andrew Clark>//
- **[72b7462fe](facebook/react@72b7462fe )**: Bump local package.json versions for 18.1 release ([#24447](facebook/react#24447)) //<Andrew Clark>//
- **[22edb9f77](facebook/react@22edb9f77 )**: React `version` field should match package.json ([#24445](facebook/react#24445)) //<Andrew Clark>//
- **[6bf3deef5](facebook/react@6bf3deef5 )**: Upgrade react-shallow-renderer to support react 18 ([#24442](facebook/react#24442)) //<Michael サイトー 中村 Bashurov>//

Changelog:
[General][Changed] - React Native sync for revisions bd4784c...d300ceb

jest_e2e[run_all_tests]

Reviewed By: cortinico, kacieb

Differential Revision: D36874368

fbshipit-source-id: c0ee015f4ef2fa56e57f7a1f6bc37dd05c949877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants