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

Dropdown value not populated on initial render w/ react 16 #1682

Closed
jaridmargolin opened this issue May 16, 2017 · 5 comments
Closed

Dropdown value not populated on initial render w/ react 16 #1682

jaridmargolin opened this issue May 16, 2017 · 5 comments

Comments

@jaridmargolin
Copy link

Steps

Expected Result

California would be selected/active

Actual Result

Placeholder is shown

Version

semantic-ui-react@0.68.3
react@16.0.0-alpha.6

Testcase

http://codepen.io/anon/pen/jmpavw


Upon further investigation it looks like semantic-ui-react is following some conventions not recommended by the react docs. Specifically I tracked the issue down to:

https://github.com/Semantic-Org/Semantic-UI-React/blob/v0.68.3/src/lib/AutoControlledComponent.js#L149

Never mutate this.state directly, as calling setState() afterwards may replace the mutation you made. Treat this.state as if it were immutable.

This turns out to be a messy problem. The dropdown is dependent on this.state being set sync (it uses this.state.value in order to find the selectedIndex), but the state needs to be set with the async setState method otherwise it gets blown out on the event loop that setState finally fires.

There may need to be a larger discussion around how controlled components behave, but I was able to side step the problem by replacing the above line with:

      this.state = _extends({}, this.state, initialAutoControlledState)
      this.setState(this.state)

The overall idea being it can take advantage of this.state being set sync but it doesn't get blown out because I also add the update to the setState queue.

@levithomason
Copy link
Member

Hm, I wouldn't think setting state directly in CWM would be much different than using an ES6 property initializer. Both set state directly, before the component mounts, and do not invoke setState. That said, it doesn't explain how your update would then solve the issue as you've noted it did.

For now, can you use defaultValue? Here's a fork of your pen showing that this indeed works: http://codepen.io/levithomason/pen/YVjLwy.

We'll revisit how the ACC is handling state, notably, initial state.

@jaridmargolin
Copy link
Author

jaridmargolin commented May 16, 2017

@levithomason - defaultValue can only be set for uncontrolled components. I am using redux-forms which works by passing the value prop in. In other words, I won't be able to use defaultValue for my use case.


Here is the block of code which explains both my code works, and why the current implementation does not... Perhaps this is a bug in react-fiber?

https://github.com/facebook/react/blob/767a5d60e8a67148dc8831de521ce636374016f1/src/renderers/shared/fiber/ReactFiberClassComponent.js#L370

Notice that the state is stored in a local variable prior to calling callComponentWillMount(workInProgress, instance);

The result is that state is null rather than the value set in CWM. This null value is then passed into beginUpdateQueue as the previousState. This explains why the initial value is overwritten.

The reason my hack works is, I make this.state available for the code that requires it to be set sync, and I additionally add it to the queue in order to mitigate the fact that it is going to be overwritten.

That should make sense? Let me know if I should or can explain any further )

@levithomason
Copy link
Member

defaultValue can only be set for uncontrolled components

defaultValue works for both controlled and uncontrolled components in SUIR. In the case of an uncontrolled Dropdown, it will set state internally from the defaultValue on initial render and manage state from there forward.

I've updated the forked pen to include two options so you can see this in action: http://codepen.io/levithomason/pen/YVjLwy

@jaridmargolin
Copy link
Author

@levithomason - I appreciate your help on this issue. I must be missing something. I see the following error:

"value" is auto controlled. Specify either defaultValue or value, but not both.

Looks like it is coming from here:

`${name} prop "${prop}" is auto controlled. Specify either ${defaultPropName} or ${prop}, but not both.`

@jaridmargolin
Copy link
Author

Can confirm that the latest fix has resolve this issue 👍

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

3 participants