[Fiber] Validate that update callback is a function

[gaearon]

Adds invariants existing in Stack.
The code is mostly copypasta from #6310.

[aweary]

Just one question, LGTM!

[aweary]

Would it make sense for this to live in ReactFiberUpdateQueue like it does in stack with ReactUpdateQueue?

[gaearon]

What would the benefit be? They're not direct equivalents, just happen to be called similarly.

[aweary]

Even if they're not direct equivalents, if validateCallback is meant to validate a callback specifically for enqueued callbacks it would make sense for it to live alongside the rest of the queue logic. Not a big deal though.

[sebmarkbage]

Consider moving the validation closer to the invocation. It ensures that the VM can possibly merge this type validation with its own validation, instead of just being dead weight. The fewer things (including function calls) that happens between related paths.

Think of it like when Flow drops its refinement. When you have something far away that might mutate a value, then the refinement isn't valid anymore so you have to reassert. Or you just move them closer to get it for free.

If we have something like:

callback();

The VM have to do a type check of callback before it does anything regardless.

Then doing something like:

if (typeof instance.render === 'function') {
  instance.render();
} else {
  throw new Error('Expected instance.render to be a function');
}

Can potentially be very cheap because the VM will already know whether it's a function or not.

Something like:

function foo(callback) {
  bar(callback);
}

May not need a type assertion because the value is only passed straight through. So if we do something like:

function foo(callback) {
  if (typeof callback === 'function') {
    throw new Error('...');
  }
  bar(callback);
}

Then we are adding an additional check here. More over, since the type information isn't preserved deeply into other functions, the refinement may get lost so there will be an additional one later on.

Even with something like:

function foo(callback) {
  assertFunction(callback);
  callback();
}

It may be difficult for this refinement to carry over unless the VM has previously inlined this, or if we can inline it with a compiler.

That's why I suggest that we should only add validation where we're doing a type check anyway. Even if that check is implicit. Not early.

[acdlite]

That makes sense. One drawback of moving validation to commitCallbacks (where the callbacks are invoked) is that we'd need to keep track where the callback came from, if we want the error message to include that information. Seems worth it, though.