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

fix 15301 15219 #15302

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Jokcy
Copy link

commented Apr 3, 2019

fix #15219 and more info in #15301

@gaearon
Copy link
Member

left a comment

Can you instead start by preparing failing test cases that this would fix? Then we can see if it we can fix it with less overhead.

Thanks!

@Jokcy

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

Sorry for my the mistake to close this issue...

@gaearon It seems I ran into some issue with react-test-renderer. code here: https://codesandbox.io/s/r58yl52o74

I recently trying to test this issue with this code:

const root = testRender.create(<App key="1" />);
root.update(<App key="1" />);

I want to trigger crash when doing this. But it never work. So I tried to get the the fiber of StatefulComponent with this code root.root.findByType(StatefulComponent) and I got it but find memoizedState just contain useState Hook.

So I tried another way, just use react-dom to trigger update using this code:

it("renders without crashing", () => {
  const div = document.createElement("div");
  ReactDOM.render(<App />, div);
  ReactDOM.render(<App />, div);
  ReactDOM.unmountComponentAtNode(div);
});

And it worked as expected, the App crashed with message Invariant Violation: Should have a queue. This is likely a bug in React. Please file an issue.

So I guess the bug is really existed but I can't test it with react-test-renderer. I may need some advice to complete this test case.

@Jokcy Jokcy closed this Apr 4, 2019

@Jokcy Jokcy reopened this Apr 4, 2019

@Jokcy

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

I also test the react-dom case in local with workInProgressHook reset at beginning and it will pass. Since I can't change node_modules code in codesandbox I can't show the example here.

@Jokcy

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

HA... I found this is not an issue of renderWithHooks😂. The reason of workInProgressHook not being reset is that React trying to replayUnitOfWork in development.

Normally without replayUnitOfWork we catch error and resetHooks at renderRoot. But when replay we run the ErrorThrow component with invokeGuardedCallback which will not resetHooks. So~~~

I guess I need to change the commit content now.

@Jokcy

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

Maybe we just need to add resetHooks after replay?

Also maybe we should set replayFailedUnitOfWorkWithInvokeGuardedCallback in test-renderer development build to true?

@Jokcy Jokcy force-pushed the Jokcy:fix-15301-and-15219 branch from 732ec61 to f4b4e5d Apr 9, 2019

@Jokcy Jokcy force-pushed the Jokcy:fix-15301-and-15219 branch from f4b4e5d to 356d8cf Apr 9, 2019

@Jokcy

This comment has been minimized.

Copy link
Author

commented Apr 9, 2019

@gaearon I push this to add resetHooks after replay. I'm waiting for some discuss about how to test this case and also if we should make replay acting same in test-renderer to test this case, since invokeGuardedCallback depend on dom event but test-renderer did not

@Jokcy Jokcy closed this Apr 9, 2019

@Jokcy Jokcy reopened this Apr 9, 2019

@Jokcy

This comment has been minimized.

Copy link
Author

commented Apr 9, 2019

I don't understand why I just comment then it will close the issue...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.