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

Anonymous components (for using hooks with less indirection) #197

Closed
wants to merge 1 commit into from

Conversation

lewisl9029
Copy link

@lewisl9029 lewisl9029 commented Jun 1, 2021

This excerpt from the summary section probably best describes the intent for this RFC.

Essentially, the anonymous components pattern aims to give users the option to use hooks in a way that doesn't force them to introduce indirection (i.e. restructure their components' internal branching, hoist variables to top of component functions, introduce new components that have to be named, prop drilled, have new types defined, etc) for the sole purposes of adhering to the rules of hooks.

The goal of the RFC at this point in time is only to unblock adoption of the userland implementation through a modification of the official React hooks linter rule. Any changes to official docs/recommendations can come if/when the userland implementation gains enough traction to be considered sufficiently battle tested.

Link to rendered version

Would really appreciate any feedback/discussion. Thanks!

@xiel
Copy link

xiel commented Jun 3, 2021

There are cases where this will not work as intended, as React will see all these "Anonymous Callback Components" as the same component, but they will have changing hooks. And by this will break with Reacts internals, that's what the eslint rules are trying to warn about.

Example 1 Breaks encapsulation, Callback A and B will share the same state.
https://codesandbox.io/s/broken-anonymous-callback-components-forked-ej0yl?file=/src/App.js

{on
        ? anonymous(() => {
            const [countA, setCountA] = useState(0);
            return (
              <div>
                <h1>A {countA}</h1>
                <button type="button" onClick={() => setCountA(countA + 1)}>
                  inc A++
                </button>
              </div>
            );
          })
        : anonymous(() => {
            const [countB, setCountB] = useState(0);
            return (
              <div>
                <h1>B {countB}</h1>
                <button type="button" onClick={() => setCountB(countB + 1)}>
                  inc B++
                </button>
              </div>
            );
          })}

Example 2 Throws, when using different hooks
Throws "Warning: React has detected a change in the order of Hooks called by Anonymous." when switching component.
https://codesandbox.io/s/broken-anonymous-callback-components-forked-hvkpg?file=/src/App.js

You might hack around this using keys, but I don't think that is the way to go.

@lewisl9029
Copy link
Author

There are cases where this will not work as intended, as React will see all these "Anonymous Callback Components" as the same component, but they will have changing hooks. And by this will break with Reacts internals, that's what the eslint rules are trying to warn about.

Example 1 Breaks encapsulation, Callback A and B will share the same state.
https://codesandbox.io/s/broken-anonymous-callback-components-forked-ej0yl?file=/src/App.js

{on
        ? anonymous(() => {
            const [countA, setCountA] = useState(0);
            return (
              <div>
                <h1>A {countA}</h1>
                <button type="button" onClick={() => setCountA(countA + 1)}>
                  inc A++
                </button>
              </div>
            );
          })
        : anonymous(() => {
            const [countB, setCountB] = useState(0);
            return (
              <div>
                <h1>B {countB}</h1>
                <button type="button" onClick={() => setCountB(countB + 1)}>
                  inc B++
                </button>
              </div>
            );
          })}

Example 2 Throws, when using different hooks
Throws "Warning: React has detected a change in the order of Hooks called by Anonymous." when switching component.
https://codesandbox.io/s/broken-anonymous-callback-components-forked-hvkpg?file=/src/App.js

You might hack around this using keys, but I don't think that is the way to go.

Yep, I do consider this a major drawback and open question since it can be very error prone in practice.

The best short term solution I can think of is a linter rule that can detect these situations and warn/auto-fix (by injecting some random key for each branch). Though I'd love to hear ideas from the community on how we can address this through the runtime API without relying on static analysis in the longer term.

@gaearon
Copy link
Member

gaearon commented Aug 19, 2021

The quick answer is you're kind of swimming against the tide here. I can understand the aesthetic argument for inlining, although I'd say that the "after" code wouldn't look so bad if it weren't interrupted by long comment blocks. :-) But the notion of an explicit component boundary is useful and important. Indeed, it's used to determine whether two components are "compatible" — even without Hooks. E.g. whether it's safe to reuse the DOM when the structure matches or not. The way the React heuristic works (based on the type comparison) is somewhat subtle but it has worked really well over the years. So overturning that needs to build a much stronger argument. Anonymous components seem nice in the beginning, but then at some point they grow too much and you need to extract them anyway... Why not do this when it's still manageable? Wanting to have some separate state seems like a reasonable time to force the extraction. Server Components (#188) also encourages more granularity. Anonymous components don't have names, which is not great for debugging. Overall, while I feel your frustration, I think you are overstating the “enormous point of friction” here. Technically, you are welcome to disable the rule and remember to add a key to every component, but it’s not something we would recommend or encourage people in the ecosystem to do.

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.

4 participants