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

Insert temporary input node to polyfill submitter argument in FormData #26714

Merged
merged 3 commits into from
Apr 24, 2023

Conversation

sebmarkbage
Copy link
Collaborator

Insert temporary input node to polyfill submitter argument in FormData. This works for buttons too and fixes a bug where the type attribute wasn't reset.

I also exclude the submitter if it's a function action. This ensures that we don't include the generated "name" when the action is a server action. Conceptually that name doesn't exist.

When I moved the other part I broke out of this validation too early.
@sebmarkbage sebmarkbage requested a review from sophiebits April 24, 2023 16:10
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 24, 2023
@sebmarkbage sebmarkbage requested a review from acdlite April 24, 2023 16:11
@react-sizebot
Copy link

react-sizebot commented Apr 24, 2023

Comparing: 2fa6323...1d7418a

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 = 163.92 kB 163.92 kB = 51.67 kB 51.67 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.06% 168.59 kB 168.70 kB +0.06% 53.01 kB 53.04 kB
facebook-www/ReactDOM-prod.classic.js +0.03% 569.79 kB 569.97 kB +0.06% 100.83 kB 100.89 kB
facebook-www/ReactDOM-prod.modern.js +0.03% 553.52 kB 553.71 kB +0.06% 98.16 kB 98.22 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 1d7418a

We also exclude the submitter if it's a function action. This ensures that
we don't include the generated "name" when the action is a server action.
@@ -53,6 +53,11 @@ function extractEvents(
if (submitterAction != null) {
// The submitter overrides the form action.
action = submitterAction;
if (typeof action === 'function') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since there is if (typeof action !== 'function') just below… skip this conditional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what I ended up doing in the replaying one. Not sure why I didn't see it here.

// TODO: FormData takes a second argument that it's the submitter but this
// is fairly new so not all browsers support it yet. Switch to that technique
// when available.
const type = submitter.type;
submitter.type = 'hidden';
const temp = submitter.ownerDocument.createElement('input');
Copy link
Collaborator

Choose a reason for hiding this comment

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

think adding .type = 'hidden' to the temp node is any more efficient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean because it won't invalidate layout or something? Not sure that's true since even hidden forms can be made to participate in layout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's also invalid on button so wouldn't make any difference in most cases.

If this is false, we'll bail anyway.
@sebmarkbage sebmarkbage merged commit 5e5342b into facebook:main Apr 24, 2023
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
facebook#26714)

Insert temporary input node to polyfill submitter argument in FormData.
This works for buttons too and fixes a bug where the type attribute
wasn't reset.

I also exclude the submitter if it's a function action. This ensures
that we don't include the generated "name" when the action is a server
action. Conceptually that name doesn't exist.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
#26714)

Insert temporary input node to polyfill submitter argument in FormData.
This works for buttons too and fixes a bug where the type attribute
wasn't reset.

I also exclude the submitter if it's a function action. This ensures
that we don't include the generated "name" when the action is a server
action. Conceptually that name doesn't exist.

DiffTrain build for commit 5e5342b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants