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

Changing size dynamically doesn't work in strict mode #309

Closed
tmeasday opened this issue Aug 9, 2018 · 8 comments
Closed

Changing size dynamically doesn't work in strict mode #309

tmeasday opened this issue Aug 9, 2018 · 8 comments

Comments

@tmeasday
Copy link
Contributor

tmeasday commented Aug 9, 2018

Describe the bug

Dynamically changing the size has no effect in React strict mode, either programatically or via devtools.

To Reproduce

Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/048norw86p
  2. Open the preview window fullscreen
  3. Open React devtools
  4. Change the size prop to some other value
  5. Notice nothing happens

Expected behavior

Changing the size prop should change the size of the fixed pane.

Additional context

As the getDerivedStateFromProps function is state dependent (it state based on the previous state), in React's strict mode it gets tripped by this sort-of zany check:

https://github.com/facebook/react/blob/067cc24f55ef01a233be9e7564b337e35fed72ea/packages/react-reconciler/src/ReactFiberClassComponent.js#L139-L148

So when you change props, getDerivedStateFromProps gets called twice and the second time, this check returns false, so newState.paneXsize is not set:

const shouldUpdateSize =
props.size !== undefined
? props.size !== instanceProps.size
: defaultSizeChanged;

I could have this explanation wrong, I'll admit it's pretty confusing.

@tmeasday
Copy link
Contributor Author

I've started trying to put a PR together to fix this but I have gotten stuck with StrictMode. Here is my progress:

https://github.com/tomkp/react-split-pane/compare/master...tmeasday:fix-309-updates-in-strict-mode?expand=1

Note this test passes if you remove the StrictMode (this is expected) but fails with the StrictMode, but not for the reason I am hoping. Instead what happens is the jsx renders to null and the test is unable to find the panel.

I have no idea why it would be that if X renders to something, <StrictMode>X</StrictMode> would render to null. Does anyone have any idea?

@ghost
Copy link

ghost commented Aug 13, 2018

Even if I do not use StrictMode, dynamic sizing in controlled components is not working..

@tmeasday
Copy link
Contributor Author

@kidkkr - do you have a reproduction? If I remove the StrictMode in my repro above it works, at least altering the values from devtools.

@ghost
Copy link

ghost commented Aug 14, 2018

Sorry. I did misunderstand situation. https://codesandbox.io/s/o9ly13qovy it works well. 😃
It seems that we use StrictMode in our product.

@wuweiweiwu
Copy link
Collaborator

I haven't had much experience with StrictMode but Ill dig around.

tmeasday added a commit to tmeasday/react-split-pane that referenced this issue Sep 8, 2018
This test is a reproduction for tomkp#309 and so is currently failing.

There is some complexity in making it work but it reproduces the problem.
tmeasday added a commit to tmeasday/react-split-pane that referenced this issue Sep 8, 2018
I'm not sure if this is the best solution for tomkp#309, as I am not entirely aware of what this code was trying to do, but it *seemed* like it was trying to avoid touching state too much which doesn't seem super useful as there wasn't any complex calculations involved -- and if there was, memoization would be more useful.

In any case it was triggering problems in strict mode which implies it would have broken in later version of React.
@tmeasday
Copy link
Contributor Author

tmeasday commented Sep 8, 2018

OK, my PR now works: #315 cc @wuweiweiwu

@wuweiweiwu
Copy link
Collaborator

wuweiweiwu commented Sep 8, 2018

Awesome! I'll take a look ASAP @tmeasday

wuweiweiwu pushed a commit that referenced this issue Sep 8, 2018
* Update to React@16.4 and add a test for strict mode

This test is a reproduction for #309 and so is currently failing.

There is some complexity in making it work but it reproduces the problem.

* Don't try and avoid updating state

I'm not sure if this is the best solution for #309, as I am not entirely aware of what this code was trying to do, but it *seemed* like it was trying to avoid touching state too much which doesn't seem super useful as there wasn't any complex calculations involved -- and if there was, memoization would be more useful.

In any case it was triggering problems in strict mode which implies it would have broken in later version of React.
@wuweiweiwu
Copy link
Collaborator

Released in 0.1.83!!

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

No branches or pull requests

2 participants