Skip to content

Conversation

@sophiebits
Copy link
Collaborator

No description provided.

@sophiebits sophiebits requested a review from sebmarkbage May 1, 2023 20:59
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 1, 2023
@react-sizebot
Copy link

react-sizebot commented May 1, 2023

Comparing: fda1f0b...fed72e3

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 = 164.19 kB 164.19 kB = 51.78 kB 51.78 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 171.55 kB 171.55 kB = 53.98 kB 53.98 kB
facebook-www/ReactDOM-prod.classic.js = 570.13 kB 570.13 kB = 100.62 kB 100.62 kB
facebook-www/ReactDOM-prod.modern.js = 553.87 kB 553.87 kB = 97.80 kB 97.80 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against fed72e3

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

This isn't limited to formActions as functions but applies to all formAction non-null values on input/button. Since we also track nesting of DOM nodes we should be able to warn about this earlier instead of waiting for the action to be invoked.

How about we do the warning in the render phase by checking DOM nesting instead?

The risk is that we don't know if you're rendering the React root into a form but we can either just assume that you're not and warn. Seems like such an edge case or check at the root.

@sophiebits
Copy link
Collaborator Author

Yeah I figured I'd not add warnings for existing code and I was thinking the non-React form container might happen. But you're right it's probably rare so maybe we just should.

@sebmarkbage
Copy link
Collaborator

Even if we add the warning for non-functions for existing code behind a flag for later, it seems nicer to get the warning earlier rather than later. Since this is something that seems like you could accidentally do even during a simple refactor.

Another interesting thing is that we don't warn for nested forms in the case where you render into one so you might be tempted to add one as a wrapper when you get this warning which wouldn't be quite correct. Although it probably works as long as it's not SSR.

@sophiebits sophiebits force-pushed the formact-warndev branch 2 times, most recently from a397c10 to ee4c4a0 Compare May 9, 2023 23:13
@sophiebits
Copy link
Collaborator Author

Weirdly we would catch invalid form nesting if the container is a form, but not if the container is inside a form. I left that logic as-is for now. I suppose since you can reparent containers that makes sense in some sense. For SSR into a form ancestor you wouldn't get the warning but you would get the hydration error, which seems fine since it should be rare.

With diffInCommitPhase it looks like the bindings currently don't have access to hostContext anytime during updates.

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