Fix setState regression

[oliviertassinari]

I have noticed the following regression after upgrade to enzyme 3.4.4.

TypeError: ReactWrapper::setState() expects a function as its second argument

I think that we are closer to how React behaves this way: https://github.com/facebook/react/blob/5031ebf6beddf88cac15f4d2c9e91f8dbb91d59d/packages/react-reconciler/src/ReactFiberClassComponent.js#L77-L92

[oliviertassinari]

Well, my fix is probably wrong, I don't have the time to look deeper, but at least you get the idea. This is fine with React, I don't think that enzyme should throw.

this.setState({}, null); // OK
this.setState({}, undefined); // OK
this.setState({}, () => {}); // OK
this.setState({}, {}); // NOT OK

[ljharb]

Why shouldn't it throw? If you're doing that in React, it's probably a bug. That React chooses not to throw is fine, but enzyme is a test framework - its job is to catch bugs.

Do you have a legitimate use case for passing a falsy non-function to either setState or setProps?

[ljharb]

if you pass undefined, the current code will replace it with noop - this would allow you to silently pass a falsy primitive (false, NaN, 0, empty string, null). What's the utility of this?

[oliviertassinari]

What about null? The default won't kick in. React checks both:
https://github.com/facebook/react/blob/5031ebf6beddf88cac15f4d2c9e91f8dbb91d59d/packages/react-reconciler/src/ReactFiberClassComponent.js#L179

function b(b = 'b') {
  return b
}

b() // 'b'
b(null) // null
b(undefined) // 'b'
b(false) // false

But I haven't poor much time in this patch, I could be wrong.

[ljharb]

You're right that null won't trigger the default; but i'm confused why you'd ever pass in a null in the first place.

[oliviertassinari]

i'm confused why you'd ever pass in a null in the first place.

Good question, I have no idea. I was curious to see how React would behave, turns out, they explicitly support it for some reason I'm not aware of.
https://github.com/facebook/react/blob/5031ebf6beddf88cac15f4d2c9e91f8dbb91d59d/packages/react-reconciler/src/ReactFiberClassComponent.js#L179

[ljharb]

I've asked for the motivation here

[ljharb]

either way, the default is required, so the .length of the function is correct.

[oliviertassinari]

enzyme is a test framework - its job is to catch bugs

How is it a bug if it's working in production?

Do you have a legitimate use case for passing a falsy non-function to either setState or setProps?

I believe I have, the callback parameter is optional in my case, it can be undefined:
https://github.com/mui-org/material-ui/blob/a29c7f1a7ee885b70159856212206e99b8e86e56/packages/material-ui/src/ButtonBase/TouchRipple.js#L248

[ljharb]

If you pass undefined with the current code, it will not throw - the default argument will replace it.

[ljharb]

It's a bug because it's unintentional - that something happens to work doesn't make it not a bug.

If you're passing "not a callback", and "not nothing", where a callback is expected, you made a mistake, plain and simple.

[oliviertassinari]

If you pass undefined with the current code, it will not throw

If you pass undefined with the current code, it will throw. arguments.length > 1 will make sure of that. It's how I have discovered the issue.
https://github.com/airbnb/enzyme/blob/3bc5de22f8b027fd6db53ac9ac3e8e5e517e6d3a/packages/enzyme/src/ShallowWrapper.js#L434

[ljharb]

Ah, you're right, I added that to setState (but not setProps).

That stricter check, however, has already caught a bug in someone's code - see #1769. I think that in this case the benefits outweigh the deviation from React in production. (Separately, I'm going to try to pursue adding the same strict check in react itself in v17)

[ljharb]

I do see, however, that because of recent changes, the wrapper's setState will be called in your component code because we stub the instance's setState. That does seem like something we could fix, in the ShallowWrapper constructor - which would allow your code to work - but it would prevent people from being able to catch bugs like in #1769.

[oliviertassinari]

Separately, I'm going to try to pursue adding the same strict check in react itself in v17

@ljharb My main purpose with this pull request was to illustrate the change of behavior. I have already patched the Material-UI code to always have a callback function defined. Personally, I would stick to React behavior as much as possible. But I can understand you have a different vision. Do what you think is best.

[ljharb]

Per the discussion; I'm going to address this by leaving the wrapper methods strict, but by making the shallow setState stub match React. This way, your code won't error, but people directly calling the methods on the wrapper will get the desired error.

[oliviertassinari]

leaving the wrapper methods strict, but by making the shallow setState stub match React.

@ljharb I haven't checked the implementation, but this behavior description sounds great, thanks :)!