Add invariant to check getInitialState return value. Fixes #397

[cpojer]

This adds an invariant that checks if getInitialState returns an object. Let me know if I should change the error message to something different.

[benjamn]

@cpojer if you want to create an upstream diff I'll stamp it

[zpao]

You say it can be null but no tests to confirm (also doesn't look like that's true)

[syranide]

@zpao typeof null === 'object' (however stupid that is)

[cpojer]

Added a test that ensures getInitialState can return null.

[fabiomcosta]

👍

[vjeux]

undefined should also be valid. Otherwise a function like this would fatal:

getInitialState: function() {
  if (something) {
    return {some: 'abc'};
  }
}

[syranide]

@vjeux Not saying it's right, but there's a merit to being explicit as well, if you define a function, you return a value (be it null). Could be used to catch mistakes. Just saying. :)

[cpojer]

Personally I'd like to be explicit. If you define a function called 'getInitialState' I think it must always return something and 'undefined' is a bad initial state. However, if this breaks a lot of code we should consider allowing it.

[vjeux]

Ok I'm sold, let's do it :)

[sebmarkbage]

TLDR; I agree.

A component that has null or undefined should be treated as a stateless component. Debugging tools may use it to determine whether this is a stateful component or a simple one. We might be able to do optimizations by knowing that a component isn't stateful.

I could see the benefit of being able to determine if a component should be stateful at runtime. That should probably not be determined by props since that always makes it potentially stateful. Class level flags you can probably just make the whole getInitialState function null if you need to.

[syranide]

@sebmarkbage Part of the discussion was about whether undefined show be considered an error or not. I would argue that all valid paths inside getInitialState should return something (and null or {} if you want it stateless) and that undefined (no return) should be considered a mistake/error.

[sebmarkbage]

Right, but perhaps we should also forbid null while we're at it?

[syranide]

@sebmarkbage I can't come up with any real scenario that would need it (and obviously none would require it). Theoretically you may want to only have a state sometimes, but again, it wouldn't require it and if you absolutely must optimize that specific case, just swap the entire function for null based on the condition.

So yeah, I'm inclined to agree with you.

[vjeux]

If you don't need any state, you can return {}; instead of null

[cpojer]

Isn't null pretty much equal to {} except that the latter allocates a new object? If we want to disallow null-states, wouldn't it make sense to disallow an empty object too? I don't really think it makes sense to differentiate between null and {} so either we allow both or none.

[vjeux]

I don't really care tbh

[cpojer]

@sebmarkbage what do you say?

[sebmarkbage]

Some people use dynamic property keys to add to the object later. Returning an object demonstrates the intension that the component is stateful. An empty object may not have all it's keys yet but will at some point.

I guess we wouldn't want to overload that intension. We should probably just allow null until we can figure out when this is used, if at all.

Don't really care. I'm just thinking out loud. You should get this is in.

[benjamn]

Upstream diff is accepted and ready to land, so I'm merging this.