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.

Sign up for GitHub

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

Jump to bottom

Add (Client) Functions as Form Actions #26674

Merged
merged 6 commits into from
Apr 19, 2023
Merged

Add (Client) Functions as Form Actions #26674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b08beb
Add feature flag
sebmarkbage Mar 10, 2023
fdecf9e
Client side form actions
sebmarkbage Apr 5, 2023
c6b06b7
Update fixture
sebmarkbage Apr 19, 2023
1d73892
Fix form field warnings
sebmarkbage Apr 19, 2023
6be6585
Grammar nits
sebmarkbage Apr 19, 2023
5a7629d
Handle the case where the submitter target is not a React element
sebmarkbage Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 17 additions & 15 deletions fixtures/flight/src/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,22 @@ export default function Button({action, children}) {
const [isPending, setIsPending] = React.useState(false);

return (
<button
disabled={isPending}
onClick={async () => {
setIsPending(true);
try {
const result = await action();
console.log(result);
} catch (error) {
console.error(error);
} finally {
setIsPending(false);
}
}}>
{children}
</button>
<form>
sebmarkbage marked this conversation as resolved.
Show resolved Hide resolved
<button
disabled={isPending}
formAction={async () => {
setIsPending(true);
try {
const result = await action();
console.log(result);
} catch (error) {
console.error(error);
} finally {
setIsPending(false);
}
}}>
{children}
</button>
</form>
);
}
4 changes: 1 addition & 3 deletions fixtures/flight/src/Form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@ export default function Form({action, children}) {

return (
<form
onSubmit={async e => {
e.preventDefault();
action={async formData => {
setIsPending(true);
try {
const formData = new FormData(e.target);
const result = await action(formData);
alert(result);
} catch (error) {
Expand Down
178 changes: 172 additions & 6 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import sanitizeURL from '../shared/sanitizeURL';
import {
enableCustomElementPropertySupport,
enableClientRenderFallbackOnTextMismatch,
enableFormActions,
enableHostSingletons,
disableIEWorkarounds,
enableTrustedTypesIntegration,
Expand All @@ -79,6 +80,10 @@ import {
let didWarnControlledToUncontrolled = false;
let didWarnUncontrolledToControlled = false;
let didWarnInvalidHydration = false;
let didWarnFormActionType = false;
let didWarnFormActionName = false;
let didWarnFormActionTarget = false;
let didWarnFormActionMethod = false;
let canDiffStyleForHydrationWarning;
if (__DEV__) {
// IE 11 parses & normalizes the style attribute as opposed to other
Expand Down Expand Up @@ -116,6 +121,102 @@ function validatePropertiesInDevelopment(type: string, props: any) {
}
}

function validateFormActionInDevelopment(
tag: string,
key: string,
value: mixed,
props: any,
) {
if (__DEV__) {
if (tag === 'form') {
if (key === 'formAction') {
console.error(
'You can only pass the formAction prop to <input> or <button>. Use the action prop on <form>.',
);
} else if (typeof value === 'function') {
if (
(props.encType != null || props.method != null) &&
!didWarnFormActionMethod
) {
didWarnFormActionMethod = true;
console.error(
'Cannot specify a encType or method for a form that specifies a ' +
'function as the action. React provides those automatically. ' +
'They will get overridden.',
);
}
if (props.target != null && !didWarnFormActionTarget) {
didWarnFormActionTarget = true;
console.error(
'Cannot specify a target for a form that specifies a function as the action. ' +
'The function will always be executed in the same window.',
);
}
}
} else if (tag === 'input' || tag === 'button') {
if (key === 'action') {
console.error(
'You can only pass the action prop to <form>. Use the formAction prop on <input> or <button>.',
);
} else if (
tag === 'input' &&
props.type !== 'submit' &&
props.type !== 'image' &&
!didWarnFormActionType
) {
didWarnFormActionType = true;
console.error(
'An input can only specify a formAction along with type="submit" or type="image".',
);
} else if (
tag === 'button' &&
props.type != null &&
props.type !== 'submit' &&
!didWarnFormActionType
) {
didWarnFormActionType = true;
console.error(
'A button can only specify a formAction along with type="submit" or no type.',
);
} else if (typeof value === 'function') {
// Function form actions cannot control the form properties
if (props.name != null && !didWarnFormActionName) {
didWarnFormActionName = true;
console.error(
'Cannot specify a "name" prop for a button that specifies a function as a formAction. ' +
'React needs it to encode which action should be invoked. It will get overridden.',
);
}
if (
(props.formEncType != null || props.formMethod != null) &&
!didWarnFormActionMethod
) {
didWarnFormActionMethod = true;
console.error(
'Cannot specify a formEncType or formMethod for a button that specifies a ' +
'function as a formAction. React provides those automatically. They will get overridden.',
);
}
if (props.formTarget != null && !didWarnFormActionTarget) {
didWarnFormActionTarget = true;
console.error(
'Cannot specify a formTarget for a button that specifies a function as a formAction. ' +
'The function will always be executed in the same window.',
);
}
}
} else {
if (key === 'action') {
console.error('You can only pass the action prop to <form>.');
} else {
console.error(
'You can only pass the formAction prop to <input> or <button>.',
);
}
}
}
}

function warnForPropDifference(
propName: string,
serverValue: mixed,
Expand Down Expand Up @@ -327,8 +428,7 @@ function setProp(
}
// These attributes accept URLs. These must not allow javascript: URLS.
case 'src':
case 'href':
case 'action':
case 'href': {
if (enableFilterEmptyStringAttributesDOM) {
if (value === '') {
if (__DEV__) {
Expand All @@ -355,8 +455,6 @@ function setProp(
break;
}
}
// Fall through to the last case which shouldn't remove empty strings.
case 'formAction': {
if (
value == null ||
typeof value === 'function' ||
Expand All @@ -377,6 +475,50 @@ function setProp(
domElement.setAttribute(key, sanitizedValue);
break;
}
case 'action':
case 'formAction': {
// TODO: Consider moving these special cases to the form, input and button tags.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to leave these here for now and not special case per tag. To keep the code smaller. Ideally these should follow the pattern of input, select and textarea instead.

That way we could more easily clear out props like target, method and encType when they're not relevant or set them to what they're supposed to be. That's what Fizz does. That probably also lead to hydration warnings but they also warn just in general.

if (
value == null ||
(!enableFormActions && typeof value === 'function') ||
typeof value === 'symbol' ||
typeof value === 'boolean'
) {
domElement.removeAttribute(key);
break;
}
if (__DEV__) {
validateFormActionInDevelopment(tag, key, value, props);
}
if (enableFormActions && typeof value === 'function') {
// Set a javascript URL that doesn't do anything. We don't expect this to be invoked
// because we'll preventDefault, but it can happen if a form is manually submitted or
// if someone calls stopPropagation before React gets the event.
// If CSP is used to block javascript: URLs that's fine too. It just won't show this
// error message but the URL will be logged.
domElement.setAttribute(
key,
// eslint-disable-next-line no-script-url
"javascript:throw new Error('" +
'A React form was unexpectedly submitted. If you called form.submit() manually, ' +
"consider using form.requestSubmit() instead. If you're trying to use " +
'event.stopPropagation() in a submit event handler, consider also calling ' +
'event.preventDefault().' +
"')",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't get minified but it doesn't have access to the shared helper anyway so we can't really do that the same way anyway. We could consider a shorter message which is what I did for SSR.

);
break;
}
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
if (__DEV__) {
checkAttributeStringCoercion(value, key);
}
const sanitizedValue = (sanitizeURL(
enableTrustedTypesIntegration ? value : '' + (value: any),
): any);
domElement.setAttribute(key, sanitizedValue);
break;
}
case 'onClick': {
// TODO: This cast may not be sound for SVG, MathML or custom elements.
if (value != null) {
Expand Down Expand Up @@ -2423,6 +2565,13 @@ function diffHydratedCustomComponent(
}
}

// This is the exact URL string we expect that Fizz renders if we provide a function action.
// We use this for hydration warnings. It needs to be in sync with Fizz. Maybe makes sense
// as a shared module for that reason.
const EXPECTED_FORM_ACTION_URL =
// eslint-disable-next-line no-script-url
"javascript:throw new Error('A React form was unexpectedly submitted.')";

function diffHydratedGenericElement(
domElement: Element,
tag: string,
Expand Down Expand Up @@ -2505,7 +2654,6 @@ function diffHydratedGenericElement(
}
case 'src':
case 'href':
case 'action':
if (enableFilterEmptyStringAttributesDOM) {
if (value === '') {
if (__DEV__) {
Expand Down Expand Up @@ -2546,11 +2694,29 @@ function diffHydratedGenericElement(
extraAttributes,
);
continue;
case 'action':
case 'formAction':
if (enableFormActions) {
const serverValue = domElement.getAttribute(propKey);
const hasFormActionURL = serverValue === EXPECTED_FORM_ACTION_URL;
if (typeof value === 'function') {
extraAttributes.delete(propKey.toLowerCase());
if (hasFormActionURL) {
// Expected
continue;
}
warnForPropDifference(propKey, serverValue, value);
continue;
} else if (hasFormActionURL) {
extraAttributes.delete(propKey.toLowerCase());
warnForPropDifference(propKey, 'function', value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: quotes in the warning :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I did it this way for now, instead of a custom warning, is that we probably want to join all the differences together in a larger diff. So this will have to change as part of that. We could pass a fake function or something to indicate how this should be rendered I guess.

continue;
}
}
hydrateSanitizedAttribute(
domElement,
propKey,
sebmarkbage marked this conversation as resolved.
Show resolved Hide resolved
'formaction',
propKey.toLowerCase(),
value,
extraAttributes,
);
Expand Down
13 changes: 13 additions & 0 deletions packages/react-dom-bindings/src/events/DOMPluginEventSystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
enableScopeAPI,
enableFloat,
enableHostSingletons,
enableFormActions,
} from 'shared/ReactFeatureFlags';
import {
invokeGuardedCallbackAndCatchFirstError,
Expand All @@ -72,6 +73,7 @@ import * as ChangeEventPlugin from './plugins/ChangeEventPlugin';
import * as EnterLeaveEventPlugin from './plugins/EnterLeaveEventPlugin';
import * as SelectEventPlugin from './plugins/SelectEventPlugin';
import * as SimpleEventPlugin from './plugins/SimpleEventPlugin';
import * as FormActionEventPlugin from './plugins/FormActionEventPlugin';

type DispatchListener = {
instance: null | Fiber,
Expand Down Expand Up @@ -173,6 +175,17 @@ function extractEvents(
eventSystemFlags,
targetContainer,
);
if (enableFormActions) {
FormActionEventPlugin.extractEvents(
dispatchQueue,
domEventName,
targetInst,
nativeEvent,
nativeEventTarget,
eventSystemFlags,
targetContainer,
);
}
}
}

Expand Down