Adjust code to handle more availability of function.name

[zpao]

As @vjeux noted in https://www.facebook.com/groups/2003630259862046/permalink/2102903916601346/, using node v6.5 fails some tests. This is due to the status quo where function.name isn't well supported. @keyanzhang point to

react/src/isomorphic/classic/class/ReactClass.js

Line 765 in ed8a753

var Constructor = function(props, context, updater) {

which is the function name we're picking up.

I changed a couple places in our code which were falling back to function.name to actually attempt to differentiate between cases where that's useful. It's not useful for the React.createClass cases since that will always be Constructor. Instead I check if we have a displayName property instead of just using truthiness which seems to be good enough to differentiate.

Tangential - we should probably consolidate so we don't duplicate this code. Originally it was pretty simple but we've had to add more checks and it's gotten longer. It's slightly different in these 2 places but close enough (and we can just use the same thing in both places).

Test Plan: Run tests in both node v6.5 & 4.x. Pass consistently.

[coveralls]

Changes Unknown when pulling 80ce391 on zpao:node-6.5-tests into * on facebook:master*.

[keyz]

.displayName?

[sophiebits]

Hrm, this feels off. It would be better to find a way to not use the .name I think. Or maybe some way to make the name not set to Constructor.

[zpao]

this feels off

I don't disagree. But I think we need .name to handle SFC and classes. Unless we want to start setting displayName ourselves when we construct these or maybe we could set Constructor.name = displayName so it's always just type.name. Alternatively, maybe we could start differentiating so we know which type of component we're dealing with and have more specific lookup paths for each.

[keyz]

IIRC function.name is a read-only property so we can't overwrite it.

[sebmarkbage]

How about:

function noName(fn) {
  return fn;
}
var Constructor = noName(function() { ... });

or some similar restructuring.

[sophiebits]

Or var Constructor = (0, function() { ... }); maybe.

[sebmarkbage]

Will the minifier preserve that?

[sebmarkbage]

I figured it'd be nice to just break out that whole thing:

createClass: function(spec) {
  return applySpec(function() { ... }, spec);
}

[zpao]

Will the minifier preserve that?

No, at least not Uglify with our current settings. But it probably doesn't matter since we only use this in warnings. And it's probably nice to have it go away in prod anyway.

As for wrapping with another function… we already have mixSpecIntoComponent which is essentially what you want. But we do a few other things to Constructor before we call that, and if we moved all of createClass into anther function it's another layer of misdirection and adding another call onto the stack, just to get around a function having a name, which seems unnecessary.

I'm going to do what Ben said with a comment explaining why.

[aweary]

What about var Constructor = true && function() { ... }? I think that should be preserved with Uglify.

[sebmarkbage]

But it probably doesn't matter since we only use this in warnings.

It matters for the React devtools since it works in prod.

I'm not too worried about adding another call on the stack since this is start up and not rerenders and start up for createClass sux anyway because of all the stuff it does in there.

[vjeux]

Yay thanks! It's now working on node 6 :)

[sebmarkbage]

Debugging in Chrome devtools is a game changer.