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

Bug: FunctionComponent re-render phase cause a bug #20675

Closed
careyke opened this issue Jan 27, 2021 · 8 comments · Fixed by #20676
Closed

Bug: FunctionComponent re-render phase cause a bug #20675

careyke opened this issue Jan 27, 2021 · 8 comments · Fixed by #20676

Comments

@careyke
Copy link

careyke commented Jan 27, 2021

React version:v17.0.1

Steps To Reproduce

There maybe be a bug in following method

function updateEffectImpl(fiberFlags, hookFlags, create, deps): void {
  const hook = updateWorkInProgressHook();
  const nextDeps = deps === undefined ? null : deps;
  let destroy = undefined;

  if (currentHook !== null) {
    const prevEffect = currentHook.memoizedState;
    destroy = prevEffect.destroy;
    if (nextDeps !== null) {
      const prevDeps = prevEffect.deps;
      if (areHookInputsEqual(nextDeps, prevDeps)) {
        pushEffect(hookFlags, create, destroy, nextDeps);
        return;
      }
    }
  }

  currentlyRenderingFiber.flags |= fiberFlags;

  hook.memoizedState = pushEffect(
    HookHasEffect | hookFlags,
    create,
    destroy,
    nextDeps,
  );
}

Link to the source code:

function updateEffectImpl(fiberFlags, hookFlags, create, deps): void {

Link to code example: https://codesandbox.io/s/reacthooks-fkihz?file=/src/MaybeABug.js

The current behavior

The first time click the button, it will not print the "effect"
But the second click will print the "effect"
the third will not, the fourth will be and so on

The expected behavior

Maybe should always print nothing!

Remarks

The reason for this phenomenon is when component entering re-render phase, the effect object is a big different in updateQueue and Hook object. But i think should be consistent。

In fact, i am not sure if it is a bug, so I look forward to receiving a reply, thanks

@careyke careyke added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jan 27, 2021
@careyke careyke changed the title Bug: Bug: FunctionComponent re-render phase cause a bug Jan 27, 2021
@eps1lon
Copy link
Collaborator

eps1lon commented Jan 27, 2021

Thanks for the report.

React version:v17.0.1

The codesandbox is using 0.0.0-experimental-4ead6b530. Could you clarify for which version you want to report the bug?

@careyke
Copy link
Author

careyke commented Jan 27, 2021

Thanks for the report.

React version:v17.0.1

The codesandbox is using 0.0.0-experimental-4ead6b530. Could you clarify for which version you want to report the bug?

I tried it in v17.0.1 and this problem still occurs, because the function updateEffectImpl has not changed.

@eps1lon
Copy link
Collaborator

eps1lon commented Jan 27, 2021

Test illustrating the current behavior on master:

it('yields inconsistently when triggering state updates during render', () => {
  let handleClick;
  function Test() {
    const [count, setCount] = useState(0);

    useEffect(() => {
      Scheduler.unstable_yieldValue(`Effect: ${count}`);
    }, [count]);

    if (count > 0) {
      setCount(0);
    }

    handleClick = () => setCount(2);

    return <Text text={`Render: ${count}`} />;
  }

  ReactNoop.act(() => {
    ReactNoop.render(<Test />);
  });

  expect(Scheduler).toHaveYielded(['Render: 0', 'Effect: 0']);

  ReactNoop.act(() => {
    handleClick();
  });

  expect(Scheduler).toHaveYielded(['Render: 0']);

  ReactNoop.act(() => {
    handleClick();
  });

  expect(Scheduler).toHaveYielded(['Render: 0', 'Effect: 0']);

  ReactNoop.act(() => {
    handleClick();
  });

  expect(Scheduler).toHaveYielded(['Render: 0']);
});

I would've expected expect(Scheduler).toHaveYielded(['Render: 0', 'Effect: 0']); to pass after every handleClick but every second click only yields ['Render: 0'] Not sure why I expected this. We only commit with unchanged deps so we should never run the effect. Yielding on every click with ['Render: 0'] is the correct answer.

I tried it in v17.0.1 and this problem still occurs, because the function updateEffectImpl has not changed.

Could you either update the issue or the codesandbox so that we avoid potential confusion when the bug is reported for one version but the repro uses a different one?

@careyke
Copy link
Author

careyke commented Jan 27, 2021

Could you either update the issue or the codesandbox so that we avoid potential confusion when the bug is reported for one version but the repro uses a different one?

yeah! i have updated the version of React in the codesandbox

@gaearon
Copy link
Collaborator

gaearon commented Jan 27, 2021

Is the behavior new in some version, or always been like this?

@gaearon gaearon removed the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jan 27, 2021
@eps1lon
Copy link
Collaborator

eps1lon commented Jan 27, 2021

Is the behavior new in some version, or always been like this?

It always has been™. Tested with a codesandbox using 16.8.

@eps1lon
Copy link
Collaborator

eps1lon commented Jan 29, 2021

@careyke Thanks for the repro! This helped a lot identifying the issue.

@careyke
Copy link
Author

careyke commented Jan 30, 2021

You are welcome! i am very happy to do this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants