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

Failing test: Switch between zero and non-zero Hooks #24438

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 25, 2022

Rendering Hooks conditionally is not supported. Linter enforces this.

However, people sometimes forget to use the linter. We should hard-error in this case. See #24391 for an example.

It looks like we hard-error when the number of Hooks changed, but not between 0 Hooks and non-0 Hooks. This adds failing tests that show the issue.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 25, 2022
@sizebot
Copy link

sizebot commented Apr 25, 2022

Comparing: bd4784c...86e6b0e

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 = 131.56 kB 131.56 kB = 42.11 kB 42.11 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.83 kB 136.83 kB = 43.69 kB 43.69 kB
facebook-www/ReactDOM-prod.classic.js = 441.15 kB 441.15 kB = 80.48 kB 80.48 kB
facebook-www/ReactDOM-prod.modern.js = 426.40 kB 426.40 kB = 78.28 kB 78.28 kB
facebook-www/ReactDOMForked-prod.classic.js = 441.15 kB 441.15 kB = 80.48 kB 80.48 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 86e6b0e

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 25, 2022

I suspect this check is the reason:

// Using memoizedState to differentiate between mount/update only works if at least one stateful hook is used.
// Non-stateful hooks (e.g. context) don't get added to memoizedState,
// so memoizedState would be null during updates and mounts.
if (__DEV__) {
if (current !== null && current.memoizedState !== null) {

Because of it, we end up using the "mount" dispatcher during an update.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 28, 2022

I did a bit of experimentation to see how the hookTypesDev logic works today.

Each group corresponds to one mount -> update case. [] means which Hooks were used. s is useState, c is useContext. mount, update, mount_with_checks corresponds to chosen dispatchers. For each group, the second line shows how hookTypesDev is filled before/after render, and whether the check is ok or error.


([]->[]):      mount([])         ->    mount([])   
               null->null                ok



([s]->[s]):     mount([s])    ->       update([s])
                 null->[s]                 ok     



([]->[s]):       mount([])              mount([s])
                null->null               null->[s] 
                                  ^^^ this should error ^^^



([s]->[]):     mount([s])           update([])
                null->[s]               ok
                               ^^^ this should error ^^^



([s]->[s,s]):      mount([s])            update([s,s])
                    null->[s]               error



([s,s]->[s]):        mount([s,s])        update([s])
                     null->[s,s]            error 



([]->[c]):         mount([])              mount([c])
                   null->null             null->[c]
                                    ^^^ this should error ^^^



([c]->[]):        mount([c])          mount_with_checks([])
                    null->[c]               ok
                                    ^^^ this should error ^^^



([s]->[s,c]):        mount([s])          update([s,c])
                    null->[s]               error



([s,c]->[s]):        mount([s,c])          update([s])
                      null->[s,c]               ok
                                        ^^^ this should error ^^^


@rickhanlonii
Copy link
Member

Possible fix here: #24535

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