lxsmnsyc / forgetti

Solve your hook spaghetti (with more spaghetti). Inspired by React Forget.
https://forgetti.vercel.app/
MIT License
352 stars 7 forks source link

Incorrect transformation of `useRef` #12

Closed SukkaW closed 1 year ago

SukkaW commented 1 year ago

Input

import { useEffect, useRef } from 'react';

export default function Example() {
  const singletonRef = useRef(null);
  if (!singletonRef.current) {
    singletonRef.current = {
      a: 0,
      b: 1,
      c: 2
    };
  }

  useEffect(() => {
    singletonRef.current.a++;
  });

  return (
    <div />
  );
}

This is a very common singleton useRef pattern: https://react.dev/reference/react/useRef#avoiding-recreating-the-ref-contents

Output

import { jsx as _jsx } from "react/jsx-runtime";
import { useMemo as _useMemo } from "react";
import { $$cache as _$$cache } from "forgetti/runtime";
import { $$equals as _$$equals } from "forgetti/runtime";
import { useEffect } from 'react';
export default function Example() {
    let _c = _$$cache(_useMemo, 6);
    let _v = _c[0] ||= {
        current: undefined
    };
    const singletonRef = _v;
    let _eq = _$$equals(_c[1], singletonRef);
    let _v2 = _eq ? _c[1] : _c[1] = singletonRef;
    let _v3 = _eq ? _c[2] : _c[2] = _v2.current;
    let _eq2 = _$$equals(_c[3], _v3);

    let _v4 = _eq2 ? _c[3] : _c[3] = _v3;
    let _v5 = _eq2 ? _c[4] : _c[4] = !_v4;
    if (_v5) {
        let _c2 = _c[5] ||= new Array(1)
        let _v6 = _c2[0] ||= {
            a: 0,
            b: 1,
            c: 2
        };
        let _v7 = _v2.current = _v6;
        _v7;
    }
    let _v8 = _c[6] ||= [];
    useEffect(()=>{
        singletonRef.current.a++;
    }, [_v8]);
    let _v9 = _c[7] ||= /*#__PURE__*/ _jsx(\\"div\\", {});
    return _v9;
}

Now singletonRef.current will always be undefined.


I propose forgetti just to skip useRef optimization. useRef is just like a global variable live within a component's lifecycle, and it is not designed to be read or written during render.

lxsmnsyc commented 1 year ago

The behavior is correct. The mistake was in your code (if (!singletonRef))

The optimization is intentional as useRef has too much overhead because of its hook implementation despite being the simplest hook

SukkaW commented 1 year ago

The behavior is correct. The mistake was in your code (if (!singletonRef))

That was a typo. I have updated the input and output sections. And yet the behavior is still incorrect.

The reference to the singletonRef is lost during the assignment, thus _v2 is not singletonRef when _v2 is coming from the cache (_c[1]). In this case, assigning _v2.current won't update singletonRef.current.

Simply put, Object.is(_v2, singletonRef) === false when _v2 is from _c[1] and _v is from _c[0];

lxsmnsyc commented 1 year ago

Pardon me as I still don't see where the mistake is. Could you point out the exact line where it loses the reference? because _v2 is always _v from the initial commit up until the component unmounts.

Even if useRef is not optimized the only code that would change is _v

SukkaW commented 1 year ago

Pardon me as I still don't see where the mistake is. Could you point out the exact line where it loses the reference? because _v2 is always _v from the initial commit up until the component unmounts.

Even if useRef is not optimized the only code that would change is _v

Anyway, I have put up a re-production at codesandbox: https://codesandbox.io/s/xenodochial-goldberg-lc5iwd?file=/src/App.tsx

image

And as you can see, the ref value is undefined during useEffect which is not supposed to happen.

lxsmnsyc commented 1 year ago

Okay thank you for making a repro, I'll take a look

lxsmnsyc commented 1 year ago

I think I understand the issue now. It's not about the useRef optimization, it's more of knowing that the cache item is fresh. For example, if we have $$equals(c[0], item) and item is undefined, and we have console.log(item), the log will never run on initial render because it thinks that c[0] already exists and the cache never changed (c[0] === undefined).

The solution here is to change the runtime comparison so that it checks whether or not there's a registered cache.