✨ Initial amp-timeago Bento component

[caroqliu]

No description provided.

[jridgewell]

mutatedAttributes callback is already integrated with PreactBaseElement. You should not override it. Instead, when it's received, we rerender the Component.

viewportCallback is to be handled inside the Component via an IntersectionObserver. See code like https://medium.com/the-non-traditional-developer/how-to-use-an-intersectionobserver-in-a-react-hook-9fb061ac6cb5

[samouri]

A nit and some questions

[caroqliu]

Current approach has the following tradeoff: Instead of setTimestamp on enter viewport, this approach calls fresh getFuzzyTimestampValue on every render -- this means render on exit viewport and with prop mutations

[dvoytenko]

This changes the timestamp on each render. And per spec it should only do so when the element returns back to the viewport, right? Why not just do it in the if (isIntersecting) {} and store the value in the state instead of isIntersecting?

[caroqliu]

I was under the impression that this approach will cost us rendering a fresh timestamp on prop mutation from amp-bind. Specifically this is the case that's important to support:

i.e. the following does nothing on prop mutation.

export function Timeago(props) {
  const [timestamp, setTimestamp] = useState(null);
  const ref = useRef(null);

  useEffect(() => {
    const node = ref.current;
    const observer = new IntersectionObserver(entries => {
      const last = entries[entries.length - 1];
      if (last.isIntersecting) {
        setTimestamp(getFuzzyTimestampValue(props));
      }
    });
    if (node) {
      observer.observe(node);
    }
    return () => observer.disconnect();
  }, []);

  useResourcesNotify();
  return createElement('time', {datetime: props['datetime'], ref}, timestamp);
}

We could update the timestamp on every render to handle prop mutation, but that both doesn't differentiate between the attempt to render on exit viewport and can also be consolidated into the approach we have now.

[samouri]

So,

• you want to update ts on either prop change or onEnterViewport
• you do not want to update ts on random parent rerender or onExitViewport.

I think you can follow Dima's suggestion, but then also have a clause like this to update on prop change:

useEffect(() => setTimestamp(...), [props.timestamp])

[jridgewell]

but then also have a clause like this to update on prop change:

See reactjs/rfcs#107 (comment), and reactjs/rfcs#107 (comment). We should not do that.

We have two cases for intersecting rerendering now:

1. false -> true
2. true -> false

If we want to make it so that we only rerender when false -> true, we need to introduce a high order state helper. I had proposed something like:

function useRerenderWhenGoingTrue(initial) {
  const ref = useRef(initial);
  const [, setCount] = useState(0);

  const setter = useCallback(newValue => {
    const oldValue = ref.current;
    if (newValue === true && oldValue === false) {
      setCount((count) => count + 1);
    }
    ref.current = newValue;
  })

  return [ref.current, setter];
}

This will cause a rerender only when inIntersecting goes from false -> true:

const [isIntersecting, setIsIntersecting] = useRerenderWhenGoingTrue(false);

const inob = new IntersectionObserver(rs => {
  setIsIntersecting(rs[0].isIntersecting);
});

[jridgewell]

But, I think the code was fine as is, it doesn't matter too much if timeago renders when exiting viewport.

[dvoytenko]

Rerunning effect on prop change is very common and no problem with it that I can think of. The "this pattern is shady" in the links, I believe, is with passing arrays around.

[dvoytenko]

Also, the initial problem wasn't with the "renders when exiting viewport", but with the content changing on every external rerender with state/props being invariant.

[dvoytenko]

This is LGTM from my side. I know this has been a very controversial topic. And perhaps we don't have to iron out all of these kinks on a single component. But the way I see it: this component does what it promises to do and it does it fairly optimally. I'd be happy to continue the discussion on this, but I think we can merge this now and adjust if we find a reason to.

[samouri]

Feel free to ignore since everyone else has approved this, but won't this create a new IntersectionObserver on any prop change? (Thats why I was thinking this should be a different useEffect)

[dvoytenko]

Yes it will. We discussed as an optimization, we can separate side-effects like you suggested or use one side-effect + useMemo to re-calculate the value. E.g.

const [intersectionCount, setIntersectionCount] = useState(0);
const timestamp = useMemo(() => {
  if (intersectionCount < 1) {
    return null;
  }
  return getFuzzyTimestamp(datetime, locale, cutoff, cutoffText);
}, [intersectionCount, datetime, locale, cutoff, cutoffText]);

useEffect(() => {
  new IntersectionObserver(() => {
    if (isIntersecting) {
      setIntersectionCount(old => old + 1);
    }
  })
}, []);

return <div>{timestamp}</div>

I think this would work, and likely a better option vs current state. It's just that we wanted to merge the first version and continue discussion with the merged code.

[samouri]

Cool. Thanks for the explanation, makes sense to me 👍