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

Auto-controlled props emit spurious warning when set to undefined (missing undefined check). #1586

Closed
DylanLukes opened this issue Apr 16, 2017 · 3 comments · Fixed by #1613

Comments

@DylanLukes
Copy link

Steps

For any auto-controlled property x, set both x and defaultX to undefined. For example, open and value in Dropdown.

Expected Result

These values should be treated as if they are undefined/do not exist.

Actual Result

Warning: Dropdown prop "open" is auto controlled. Specify either defaultOpen or open, but not both.

Version

0.68.0

Testcase

http://codepen.io/anon/pen/xdGPxz?editors=1111

Suggested Fix

Here in AutoControlledComponent.js, replace:

  if (defaultPropName in this.props && prop in this.props) { ... }

by

  if (_.has(this.props, defaultPropName) 
      && !_.isUndefined(this.props[defaultPropName])  
      && _.has(this.props, prop) 
      && !_.isUndefined(this.props[props]) {
    ...
  }
@DylanLukes
Copy link
Author

DylanLukes commented Apr 16, 2017

In case anyone is wondering, "why on earth would you set open and defaultOpen to undefined?"...

The answer is interop with other languages targeting JS. For example, using ScalaJS with a Props facade type. When constructing an instance of Props, all of the factory/constructor arguments must be specified or defaulted to js.undefined, as there is no concept of an "unset" attribute.

@levithomason
Copy link
Member

levithomason commented Apr 18, 2017

At first glance, I think this will still pass tests and be backward compatible. In fact, we likely should've been treating undefined as not present to begin with.

Let's get a PR and see what happens. Note, the logic can be reduced to this as we only care that they are undefined, not that they are present at all:

if (!_.isUndefined(this.props[defaultPropName]) && !_.isUndefined(this.props[prop]) {
  ...
}

@jonmajorc
Copy link
Contributor

jonmajorc commented Apr 23, 2017

Hey @levithomason and @DylanLukes I am looking to contribute. I am guessing this issue is still open and is needing to be done, so I would love to help! This is also my first open source contribution too.

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