fix(view): fix the forwardRef functionality for view

[cristianbote]

Resolves frontity/frontity#773

Proposed changes:
This one fixes the forwardRef usage together with the view components.

This was part of our pair programming session @luisherranz. Let me know if I borked something up 😄

[luisherranz]

The code looks great to me, but for some reason it's not rerendering when the store changes. I have no idea why 😅

https://www.loom.com/share/bc4f1a5ec31a4bdfbbd5bc6cef96ce83

[cristianbote]

The code looks great to me, but for some reason it's not rerendering when the store changes. I have no idea why 😅

https://www.loom.com/share/bc4f1a5ec31a4bdfbbd5bc6cef96ce83

I've recorded this reply https://www.loom.com/share/b98cdb54e51d45c6b024a0f1465e68f8

TL;DR; at the end of the day, forwardRef wraps the memoed render function but it should be the other way around. Still investigating it.

[luisherranz]

How bad is to do forwardRef(memo(forwardRef(Comp)))? Or that doesn't work either?

[cristianbote]

How bad is to do forwardRef(memo(forwardRef(Comp)))? Or that doesn't work either?

That doesn't work either. I mean that's what we do right now.

Yesterday though, I went on anther direction with it, and I think it's all related to how observer-util tracks the properties usages. Basically doesn't pick-up that the forwarded inner component uses the store and needs to be updated. Is there some functionality that will ensure/enforce a view() component to render based on a store change?

[luisherranz]

Is there some functionality that will ensure/enforce a view() component to render based on a store change?

I'm not sure what you mean by that.

The way it works is by wrapping the component in an observe function, which is the computation.

The useMemo is used to avoid creating a new observe function on each rerender.

The computation tracks all the reads done during the computation execution. This should work just fine:

observe(
  (() => {
    (() => {
      state.some.prop;
    })();
  })()
);

The rerender is triggered by the setState({}) call. Maybe that's where the problem is?

[luisherranz]

Now that we use a reference (reaction.current) I've tried removing the useMemo but it doesn't help either.

EDIT: This is what I was trying.

ReactiveComp = (props, ref) => {
  // use a dummy setState to update the component
  const [, setState] = useState();
  // use a ref to store the reaction
  const reaction = useRef(null);

  // create a reactive wrapper of the original component (render)
  // at the very first run of the component function
  if (!reaction.current) {
    reaction.current = observe(Comp, {
      scheduler: () => {
        // trigger a new rerender if the component has been mounted
        if (reaction.current.mounted) setState({});
        // mark it as changed if the component has not been mounted yet
        else reaction.current.changedBeforeMounted = true;
      },
      lazy: true,
    });
    // initilalize a flag to know if the component was finally mounted
    reaction.current.mounted = false;
    // initilalize a flag to know if the was reaction was invalidated
    // before the component was mounted
    reaction.current.changedBeforeMounted = false;
  }

  useEffect(() => {
    // mark the component as mounted.
    reaction.current.mounted = true;

    // if there was a change before the component was mounted, trigger a
    // new rerender
    if (reaction.current.changedBeforeMounted) setState({});

    // cleanup the reactive connections after the very last render of the
    return () => unobserve(reaction.current);
  }, []);

  // run the reactive render instead of the original one
  return isForwardRef
    ? forwardRef(reaction.current(props, ref))
    : reaction.current(props);
};

[michalczaplinski]

Additional thing to consider is that a forwardRef component is an object with a render() method, unlike a class / function components which are just functions:

import { Component, forwardRef } from "react";

class ClassComp extends Component {
  render() {
    return <div>I am the Foo</div>;
  }
}

const FuncComp = () => <div> hello </div>;
const Fwd = forwardRef((props, ref) => <div ref={ref} {...props} />);

console.log(typeof ClassComp); // 'function'
console.log(typeof FuncComp);  // 'function'
console.log(typeof Fwd);       // 'object'

There is known bug which is that runAsReaction does not handle forwardRef components correctly. It will try to to call Reflect.apply(fn, context, args) and assume that fn is a function when it is a forwardRef component.

I was able to get around it by changing this line to:

// check if the fn is actually a forward_ref object and in that case call its `render` method
if (typeof fn === 'object' && 
    fn !== null && 
    fn.$$typeof === Symbol.for('react.forward_ref')
) {
  return Reflect.apply(fn.render, context, args);
}
return Reflect.apply(fn, context, args);

However, I did not dig deep enough to be 100% certain that it's the complete solution.

PS. This should be fixed in https://github.com/frontity/observer-util, but I'm documenting this here for completeness.

[luisherranz]

For context, Cris is trying to fix this in: frontity/observer-util#2

I asked him if we could do this in react-easy-state instead, because observer-util is framework agnostic.

[luisherranz]

Hey guys, what is the status of this? Any progress? 🙂