@samitouri / QOS-React / commits / 2fa6323818

Restore server controlled form fields to whatever they should be (#26708)

Fizz can emit whatever it wants for the SSR version of these fields when it's a function action so they might not align with what is in the previous props. Therefore we need to force them to update if we're updating to a non-function where they might be relevant again.

Sebastian Markbåge committed Apr 23, 2023 at 17:44 UTC 2fa632381839c8732dad9107b90911163b7f2b7a

3 files changed +283 -28

packages/react-dom-bindings/src/client/ReactDOMComponent.js

+118 -27

@@ -483,6 +483,68 @@ function setProp(

483 case 'action':

484 case 'formAction': {

485 // TODO: Consider moving these special cases to the form, input and button tags.

486 + if (enableFormActions) {

487 + if (typeof value === 'function') {

488 + // Set a javascript URL that doesn't do anything. We don't expect this to be invoked

489 + // because we'll preventDefault, but it can happen if a form is manually submitted or

490 + // if someone calls stopPropagation before React gets the event.

491 + // If CSP is used to block javascript: URLs that's fine too. It just won't show this

492 + // error message but the URL will be logged.

493 + domElement.setAttribute(

494 + key,

495 + // eslint-disable-next-line no-script-url

496 + "javascript:throw new Error('" +

497 + 'A React form was unexpectedly submitted. If you called form.submit() manually, ' +

498 + "consider using form.requestSubmit() instead. If you're trying to use " +

499 + 'event.stopPropagation() in a submit event handler, consider also calling ' +

500 + 'event.preventDefault().' +

501 + "')",

502 + );

503 + break;

504 + } else if (typeof prevValue === 'function') {

505 + // When we're switching off a Server Action that was originally hydrated.

506 + // The server control these fields during SSR that are now trailing.

507 + // The regular diffing doesn't apply since we compare against the previous props.

508 + // Instead, we need to force them to be set to whatever they should be now.

509 + // This would be a lot cleaner if we did this whole fork in the per-tag approach.

510 + if (key === 'formAction') {

511 + if (tag !== 'input') {

512 + // Setting the name here isn't completely safe for inputs if this is switching

513 + // to become a radio button. In that case we let the tag based override take

514 + // control.

515 + setProp(domElement, tag, 'name', props.name, props, null);

516 + }

517 + setProp(

518 + domElement,

519 + tag,

520 + 'formEncType',

521 + props.formEncType,

522 + props,

523 + null,

524 + );

525 + setProp(

526 + domElement,

527 + tag,

528 + 'formMethod',

529 + props.formMethod,

530 + props,

531 + null,

532 + );

533 + setProp(

534 + domElement,

535 + tag,

536 + 'formTarget',

537 + props.formTarget,

538 + props,

539 + null,

540 + );

541 + } else {

542 + setProp(domElement, tag, 'encType', props.encType, props, null);

543 + setProp(domElement, tag, 'method', props.method, props, null);

544 + setProp(domElement, tag, 'target', props.target, props, null);

545 + }

546 + }

547 + }

548 if (

549 value == null ||

550 (!enableFormActions && typeof value === 'function') ||

@@ -495,24 +557,6 @@ function setProp(

557 if (__DEV__) {

558 validateFormActionInDevelopment(tag, key, value, props);

559 }

498 - if (enableFormActions && typeof value === 'function') {

499 - // Set a javascript URL that doesn't do anything. We don't expect this to be invoked

500 - // because we'll preventDefault, but it can happen if a form is manually submitted or

501 - // if someone calls stopPropagation before React gets the event.

502 - // If CSP is used to block javascript: URLs that's fine too. It just won't show this

503 - // error message but the URL will be logged.

504 - domElement.setAttribute(

505 - key,

506 - // eslint-disable-next-line no-script-url

507 - "javascript:throw new Error('" +

508 - 'A React form was unexpectedly submitted. If you called form.submit() manually, ' +

509 - "consider using form.requestSubmit() instead. If you're trying to use " +

510 - 'event.stopPropagation() in a submit event handler, consider also calling ' +

511 - 'event.preventDefault().' +

512 - "')",

513 - );

514 - break;

515 - }

560 // `setAttribute` with objects becomes only `[object]` in IE8/9,

561 // ('' + value) makes it output the correct toString()-value.

562 if (__DEV__) {

@@ -1138,7 +1182,7 @@ export function setInitialProperties(

1182 break;

1183 }

1184 default: {

1141 - setProp(domElement, tag, propKey, propValue, props);

1185 + setProp(domElement, tag, propKey, propValue, props, null);

1186 }

1187 }

1188 }

@@ -1169,7 +1213,7 @@ export function setInitialProperties(

1213 break;

1214 }

1215 default: {

1172 - setProp(domElement, tag, propKey, propValue, props);

1216 + setProp(domElement, tag, propKey, propValue, props, null);

1217 }

1218 }

1219 }

@@ -1935,7 +1979,14 @@ export function updatePropertiesWithDiff(

1979 break;

1980 }

1981 default: {

1938 - setProp(domElement, tag, propKey, propValue, nextProps, null);

1982 + setProp(

1983 + domElement,

1984 + tag,

1985 + propKey,

1986 + propValue,

1987 + nextProps,

1988 + lastProps[propKey],

1989 + );

1990 }

1991 }

1992 }

@@ -2010,7 +2061,14 @@ export function updatePropertiesWithDiff(

2061 }

2062 // defaultValue are ignored by setProp

2063 default: {

2013 - setProp(domElement, tag, propKey, propValue, nextProps, null);

2064 + setProp(

2065 + domElement,

2066 + tag,

2067 + propKey,

2068 + propValue,

2069 + nextProps,

2070 + lastProps[propKey],

2071 + );

2072 }

2073 }

2074 }

@@ -2045,7 +2103,14 @@ export function updatePropertiesWithDiff(

2103 }

2104 // defaultValue is ignored by setProp

2105 default: {

2048 - setProp(domElement, tag, propKey, propValue, nextProps, null);

2106 + setProp(

2107 + domElement,

2108 + tag,

2109 + propKey,

2110 + propValue,

2111 + nextProps,

2112 + lastProps[propKey],

2113 + );

2114 }

2115 }

2116 }

@@ -2066,7 +2131,14 @@ export function updatePropertiesWithDiff(

2131 break;

2132 }

2133 default: {

2069 - setProp(domElement, tag, propKey, propValue, nextProps, null);

2134 + setProp(

2135 + domElement,

2136 + tag,

2137 + propKey,

2138 + propValue,

2139 + nextProps,

2140 + lastProps[propKey],

2141 + );

2142 }

2143 }

2144 }

@@ -2105,7 +2177,14 @@ export function updatePropertiesWithDiff(

2177 }

2178 // defaultChecked and defaultValue are ignored by setProp

2179 default: {

2108 - setProp(domElement, tag, propKey, propValue, nextProps, null);

2180 + setProp(

2181 + domElement,

2182 + tag,

2183 + propKey,

2184 + propValue,

2185 + nextProps,

2186 + lastProps[propKey],

2187 + );

2188 }

2189 }

2190 }

@@ -2122,7 +2201,7 @@ export function updatePropertiesWithDiff(

2201 propKey,

2202 propValue,

2203 nextProps,

2125 - null,

2204 + lastProps[propKey],

2205 );

2206 }

2207 return;

@@ -2134,7 +2213,7 @@ export function updatePropertiesWithDiff(

2213 for (let i = 0; i < updatePayload.length; i += 2) {

2214 const propKey = updatePayload[i];

2215 const propValue = updatePayload[i + 1];

2137 - setProp(domElement, tag, propKey, propValue, nextProps, null);

2216 + setProp(domElement, tag, propKey, propValue, nextProps, lastProps[propKey]);

2217 }

2218 }

2219

@@ -2709,6 +2788,18 @@ function diffHydratedGenericElement(

2788 const hasFormActionURL = serverValue === EXPECTED_FORM_ACTION_URL;

2789 if (typeof value === 'function') {

2790 extraAttributes.delete(propKey.toLowerCase());

2791 + // The server can set these extra properties to implement actions.

2792 + // So we remove them from the extra attributes warnings.

2793 + if (propKey === 'formAction') {

2794 + extraAttributes.delete('name');

2795 + extraAttributes.delete('formenctype');

2796 + extraAttributes.delete('formmethod');

2797 + extraAttributes.delete('formtarget');

2798 + } else {

2799 + extraAttributes.delete('enctype');

2800 + extraAttributes.delete('method');

2801 + extraAttributes.delete('target');

2802 + }

2803 if (hasFormActionURL) {

2804 // Expected

2805 continue;

packages/react-dom-bindings/src/client/ReactDOMInput.js

-1

@@ -131,7 +131,6 @@ export function updateInput(

131 // Submit/reset inputs need the attribute removed completely to avoid

132 // blank-text buttons.

133 node.removeAttribute('value');

134 - return;

134 }

135

136 if (disableInputAttributeSyncing) {

packages/react-dom/src/__tests__/ReactDOMFizzForm-test.js

+165

@@ -195,4 +195,169 @@ describe('ReactDOMFizzForm', () => {

195 'Prop `action` did not match. Server: "action" Client: "function action(formData) {}"',

196 );

197 });

198 +

199 + // @gate enableFormActions || !__DEV__

200 + it('should reset form fields after you update away from hydrated function', async () => {

201 + const formRef = React.createRef();

202 + const inputRef = React.createRef();

203 + const buttonRef = React.createRef();

204 + function action(formData) {}

205 + function App({isUpdate}) {

206 + return (

207 + <form

208 + action={isUpdate ? 'action' : action}

209 + ref={formRef}

210 + method={isUpdate ? 'POST' : null}>

211 + <input

212 + type="submit"

213 + formAction={isUpdate ? 'action' : action}

214 + ref={inputRef}

215 + formTarget={isUpdate ? 'elsewhere' : null}

216 + />

217 + <button

218 + formAction={isUpdate ? 'action' : action}

219 + ref={buttonRef}

220 + formEncType={isUpdate ? 'multipart/form-data' : null}

221 + />

222 + </form>

223 + );

224 + }

225 +

226 + const stream = await ReactDOMServer.renderToReadableStream(<App />);

227 + await readIntoContainer(stream);

228 + let root;

229 + await act(async () => {

230 + root = ReactDOMClient.hydrateRoot(container, <App />);

231 + });

232 + await act(async () => {

233 + root.render(<App isUpdate={true} />);

234 + });

235 + expect(formRef.current.getAttribute('action')).toBe('action');

236 + expect(formRef.current.hasAttribute('encType')).toBe(false);

237 + expect(formRef.current.getAttribute('method')).toBe('POST');

238 + expect(formRef.current.hasAttribute('target')).toBe(false);

239 +

240 + expect(inputRef.current.getAttribute('formAction')).toBe('action');

241 + expect(inputRef.current.hasAttribute('name')).toBe(false);

242 + expect(inputRef.current.hasAttribute('formEncType')).toBe(false);

243 + expect(inputRef.current.hasAttribute('formMethod')).toBe(false);

244 + expect(inputRef.current.getAttribute('formTarget')).toBe('elsewhere');

245 +

246 + expect(buttonRef.current.getAttribute('formAction')).toBe('action');

247 + expect(buttonRef.current.hasAttribute('name')).toBe(false);

248 + expect(buttonRef.current.getAttribute('formEncType')).toBe(

249 + 'multipart/form-data',

250 + );

251 + expect(buttonRef.current.hasAttribute('formMethod')).toBe(false);

252 + expect(buttonRef.current.hasAttribute('formTarget')).toBe(false);

253 + });

254 +

255 + // @gate enableFormActions || !__DEV__

256 + it('should reset form fields after you remove a hydrated function', async () => {

257 + const formRef = React.createRef();

258 + const inputRef = React.createRef();

259 + const buttonRef = React.createRef();

260 + function action(formData) {}

261 + function App({isUpdate}) {

262 + return (

263 + <form action={isUpdate ? undefined : action} ref={formRef}>

264 + <input

265 + type="submit"

266 + formAction={isUpdate ? undefined : action}

267 + ref={inputRef}

268 + />

269 + <button formAction={isUpdate ? undefined : action} ref={buttonRef} />

270 + </form>

271 + );

272 + }

273 +

274 + const stream = await ReactDOMServer.renderToReadableStream(<App />);

275 + await readIntoContainer(stream);

276 + let root;

277 + await act(async () => {

278 + root = ReactDOMClient.hydrateRoot(container, <App />);

279 + });

280 + await act(async () => {

281 + root.render(<App isUpdate={true} />);

282 + });

283 + expect(formRef.current.hasAttribute('action')).toBe(false);

284 + expect(formRef.current.hasAttribute('encType')).toBe(false);

285 + expect(formRef.current.hasAttribute('method')).toBe(false);

286 + expect(formRef.current.hasAttribute('target')).toBe(false);

287 +

288 + expect(inputRef.current.hasAttribute('formAction')).toBe(false);

289 + expect(inputRef.current.hasAttribute('name')).toBe(false);

290 + expect(inputRef.current.hasAttribute('formEncType')).toBe(false);

291 + expect(inputRef.current.hasAttribute('formMethod')).toBe(false);

292 + expect(inputRef.current.hasAttribute('formTarget')).toBe(false);

293 +

294 + expect(buttonRef.current.hasAttribute('formAction')).toBe(false);

295 + expect(buttonRef.current.hasAttribute('name')).toBe(false);

296 + expect(buttonRef.current.hasAttribute('formEncType')).toBe(false);

297 + expect(buttonRef.current.hasAttribute('formMethod')).toBe(false);

298 + expect(buttonRef.current.hasAttribute('formTarget')).toBe(false);

299 + });

300 +

301 + // @gate enableFormActions || !__DEV__

302 + it('should restore the form fields even if they were incorrectly set', async () => {

303 + const formRef = React.createRef();

304 + const inputRef = React.createRef();

305 + const buttonRef = React.createRef();

306 + function action(formData) {}

307 + function App({isUpdate}) {

308 + return (

309 + <form

310 + action={isUpdate ? 'action' : action}

311 + ref={formRef}

312 + method="DELETE">

313 + <input

314 + type="submit"

315 + formAction={isUpdate ? 'action' : action}

316 + ref={inputRef}

317 + formTarget="elsewhere"

318 + />

319 + <button

320 + formAction={isUpdate ? 'action' : action}

321 + ref={buttonRef}

322 + formEncType="text/plain"

323 + />

324 + </form>

325 + );

326 + }

327 +

328 + // Specifying the extra form fields are a DEV error, but we expect it

329 + // to eventually still be patched up after an update.

330 + await expect(async () => {

331 + const stream = await ReactDOMServer.renderToReadableStream(<App />);

332 + await readIntoContainer(stream);

333 + }).toErrorDev([

334 + 'Cannot specify a encType or method for a form that specifies a function as the action.',

335 + 'Cannot specify a formTarget for a button that specifies a function as a formAction.',

336 + ]);

337 + let root;

338 + await expect(async () => {

339 + await act(async () => {

340 + root = ReactDOMClient.hydrateRoot(container, <App />);

341 + });

342 + }).toErrorDev(['Prop `formTarget` did not match.']);

343 + await act(async () => {

344 + root.render(<App isUpdate={true} />);

345 + });

346 + expect(formRef.current.getAttribute('action')).toBe('action');

347 + expect(formRef.current.hasAttribute('encType')).toBe(false);

348 + expect(formRef.current.getAttribute('method')).toBe('DELETE');

349 + expect(formRef.current.hasAttribute('target')).toBe(false);

350 +

351 + expect(inputRef.current.getAttribute('formAction')).toBe('action');

352 + expect(inputRef.current.hasAttribute('name')).toBe(false);

353 + expect(inputRef.current.hasAttribute('formEncType')).toBe(false);

354 + expect(inputRef.current.hasAttribute('formMethod')).toBe(false);

355 + expect(inputRef.current.getAttribute('formTarget')).toBe('elsewhere');

356 +

357 + expect(buttonRef.current.getAttribute('formAction')).toBe('action');

358 + expect(buttonRef.current.hasAttribute('name')).toBe(false);

359 + expect(buttonRef.current.getAttribute('formEncType')).toBe('text/plain');

360 + expect(buttonRef.current.hasAttribute('formMethod')).toBe(false);

361 + expect(buttonRef.current.hasAttribute('formTarget')).toBe(false);

362 + });

363 });