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

Remove false positive warning and add TODOs about current being non-null #14821

Merged
merged 3 commits into from Feb 13, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Feb 11, 2019

Failing test for #14790.

Seems like we expect that alternate is always empty for Hook "mounts" but that's not the cause if a memo'd component has previously suspended.

Not sure what idiomatic fix is.

@sebmarkbage
Copy link
Collaborator

.alternate should always be null for true mounts. Something may get upgraded but in that case it should disconnect the .alternate. So I think something else is broken if that's not the case.

@gaearon
Copy link
Collaborator Author

gaearon commented Feb 11, 2019

Plain function:

First attempt
- mount indeterminate
  - throws
  - (stays indeterminate)

Second attempt
- mount indeterminate
  - current != null so resets alternate // <-- this is why there's no false positive for memo()-less components
  - gets tagged as functional

Memo:

First attempt
- update memo
  - gets tagged as simple
  - updates simple
    - throws

Second attempt
- updates simple
  - false positive <--- alternate wasn't reset

@gaearon
Copy link
Collaborator Author

gaearon commented Feb 11, 2019

It seems like we use something else as an indicator whether we were suspended and should disconnect the alternate. For classes, we check the instance. For functions, we know we suspended because we're still Indeterminate. But for memo, we don't seem to use any indicator.

@gaearon
Copy link
Collaborator Author

gaearon commented Feb 11, 2019

Actually looks like classes have a special IncompleteClassComponent tag for this.

@acdlite
Copy link
Collaborator

acdlite commented Feb 11, 2019

We should be able to remove this warning entirely, now that we have this invariant:

// Clone from the current hook.
invariant(
nextCurrentHook !== null,
'Rendered more hooks than during the previous render.',
);
currentHook = nextCurrentHook;

I think I meant to do this in my PR but I must have forgotten.

@gaearon gaearon changed the title Failing test for false positive warning Remove false positive warning and add TODOs about current being non-null Feb 12, 2019
This was referenced Sep 20, 2019
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.

None yet

4 participants