react-stand-in incorrectly picks up Relay containers as functional components

[taion]

This is against v4 beta 13.

Relay Classic uses an odd pattern to lazily build containers: https://github.com/facebook/relay/blob/d77a34c6b5abddfe078b0d50c2d43181fe35d574/packages/react-relay/classic/container/RelayContainer.js#L1050-L1055

This behaves as a normal class, but does not get picked up as a React class here.

[theKashey]

Ok, so it uses a bit uncommon feature, about returning another class instance from a constructor.

[taion]

Yeah – let me see if I can monkeypatch my way out of this. This is sort of a stupid edge case anyway.

[theKashey]

It is not a simple to fix this stuff, as long you will have to hot-patch a Component returned from RelayContainer, which may be tricky with ES6 classes.
But we can not wrap such constructors at all, just to not break anything.

[taion]

This appears to work:

import Relay from 'react-relay/classic';

if (__DEV__) {
  const createRelayContainer = Relay.createContainer;

  Relay.createContainer = (...args) => {
    const Container = createRelayContainer(...args);
    Container.prototype.isReactComponent = {};

    return Container;
  };
}

Can you reliably distinguish such constructors from normal functional components though? They seem to look much the same from the outside.

[aminland]

@taion can you try this: #776

My issue was an error being thrown on Relay Modern, not classic. I spent the better part of the last 6 hours trying to figure out wtf was going on lol... I'm think the same hack might fix both though...

[theKashey]

@aminland - no, that's not a solution.
I am looking forward to create isComponentWrappable function, if not - just dont proxy.
We actually had that logic before, and dont wrap anything inside node_modules, but next decide that it is doable.
It is doable expect returning another instance from a constructor.
So

const isComponentWrappable (BaseClass) => {
  return testES5(BaseClass) || testES6(BaseClass);
}
const testES5 = (baseClass) => {
  function Tester (){
     const that = baseClass.call(this);
     return that==this;
  }  
  try{
    return !!(new Tester());
  } catch(e) {
    return false; // it was ES6 class
  }
}

const testES6 = (baseClass) => {
   class Tester extends baseClass {
      constructor(props) {
         return super(props);
      }
      MAGIC_FUNCTION(){}
   }
  try{
    return !!(new Tester()). MAGIC_FUNCTION
  } catch(e){
    return false;
  }
} 

But this will fail for Redux as long react-redux's connect require a store passed down via props or context. If it will not find it - it will throw an error.

So we can't use this code to probe.

[aminland]

So I think this is actually the right way to do it. See here: https://github.com/facebook/react/blob/e6e393b9c5221bfb1a5ddcc7221c42e96ab3baca/packages/react-reconciler/src/ReactFiberBeginWork.js#L476-L487

You can see that the reason you can mount simple functions that return component classes and not actual elements, is because if there is no Component.prototype.isReactClass set, React will first call it, then check if it just an object with a render func, in which case it will proceed with the regular lifecycle.

Since we're wrapping all non-compontents as components, any component factories that normally work in react will proceed to break, since react will no-longer check for the render method... Let me know if that makes sense...

[theKashey]

@aminland - good catch. Look like it is possible to return pre-created instance only from IndeterminateComponent(got no idea what does it mean).
It is impossible to return it from any other place.

Actually - I could not find any documentation about this. @gaearon - could you spill some light?

[taion]

I amended the issue title. It turns out Relay Modern also uses this approach for its containers. Also, Relay Modern is not so easy to monkey-patch in the same way because of the way it does its exports.

[theKashey]

Fix merged, but the update is not published yet.

[theKashey]

Released in 4.0.0-beta.14

[taion]

Thanks!

[theKashey]

@taion , @aminland - please have a look on #833