facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.93k stars 47.13k forks source link

`useCallback()` & `useMemo()` automatically with a Babel Plug-in #14658

Open DAB0mB opened 5 years ago

DAB0mB commented 5 years ago

Do you want to request a feature or report a bug?

Feature request.

What is the current behavior?

We need to useCallback() and useMemo() which seems redundant and can cost us in performance if not used right, which is likely to happen.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

export default ({ data, sortComparator, filterPredicate, history }) => {
  const transformedData = data.filter(filterPredicate).sort(sortComparator)

  return (
    <div>
      <button className="back-btn" onClick={() => history.pop()} />
      <ul className="data-list">
        {transformedData.map(({ id, value }) => (
          <li className="data-item" key={id} onClick={() => history.push(`data/${id}`)}>{value}</li>
        ))}
      </ul>
    </div>
  )
}

What is the expected behavior?

Just like the docs suggest:

In the future, a sufficiently advanced compiler could create this array automatically.

Accordingly, I have implemented a Babel-plug-in that does exactly that; see babel-plugin-react-persist. Given the code snippet above, the plug-in should generate the following output:

let _anonymousFnComponent, _anonymousFnComponent2

export default ({ data, sortComparator, filterPredicate, history }) => {
  const transformedData = React.useMemo(() =>
    data.filter(filterPredicate).sort(sortComparator)
  , [data, data.filter, filterPredicate, sortComparator])

  return React.createElement(_anonymousFnComponent2 = _anonymousFnComponent2 || (() => {
    const _onClick2 = React.useCallback(() => history.pop(), [history, history.pop])

    return (
      <div>
        <button className="back-btn" onClick={_onClick2} />
        <ul className="data-list">
          {transformedData.map(({ id, value }) =>
            React.createElement(_anonymousFnComponent = _anonymousFnComponent || (() => {
              const _onClick = React.useCallback(() =>
                history.push(`data/${id}`)
              , [history, history.push, id])

              return (
                <li className="data-item" key={id} onClick={_onClick}>
                  {value}
                </li>
              )
            }), { key: id })
          )}
        </ul>
      </div>
    )
  }), null)
}

The plug-in will:

I don't see however how can useEffect() be inferred automatically and if it's a good idea. The plug-in is not a feature request directly for React, but since it's stated in the docs I thought maybe it can be useful somehow. Maybe it can potentially be included as part of create-react-app? Would like to hear your thoughts about it. An alternative solution is suggested at #14406, but I don't see why do this at runtime when everything can be done ahead of time and save processing power.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.8-alpha (hooks)

yuchi commented 5 years ago

I’m sorry but I think you misread the content of the issue #14406. No one is talking about runtime checks. Indeed I created hooks.macro which does the job at build time — it looks at runtime, but that’s the power of Babel macros.

DAB0mB commented 5 years ago

@yuchi Your'e right I misread it. However, why do we need to use hooks like useAutoMemo if it can be inferred without it? Also what are your thoughts about the optimization with anonymous callbacks? This has been a problem for a long time with React components, and with hooks this can be solved. I don't see why we shouldn't take advantage of that.

jedwards1211 commented 5 years ago

@DAB0mB super cool! I was thinking it would be even better if it could wrap the transformedData.map in a useMemo call -- seems like that should be possible, do you see any potential obstacles to it?

DAB0mB commented 5 years ago

@jedwards1211 Everything's possible. Could you give me a sample input/output because I'm not sure exactly what do you mean. Also try to look at @yuchi's implementation, maybe you'll find something interesting.

jedwards1211 commented 5 years ago

@DAB0mB yeah, actually I put some examples at https://github.com/DAB0mB/babel-plugin-react-persist/issues/1

jedwards1211 commented 5 years ago

Maybe at some point I could contribute a PR myself

yuchi commented 5 years ago

Ok, it took some time before I could look deeply at this proposal. The approach is very intersting IMHO, your issue description doesn’t explain all the optimiziations introduced.

So, let me share some ideas:

  1. First of all, it looks a little bit too aggressive: how does it recognize a component from a normal arrow? This is unclear and potentially confusing. IMHO with a Babel Macro you could offer an API that explicitly marks (by wrapping) components for deep optimizations.
  2. Also having it as a Babel plugin makes it very diffult to try it out on Create React App projects. Again, a Babel Macro would solve this.
  3. Creating “virtual” intermediate components on the fly is a great idea that should be explored more. One problem I’m thinking about is that it can‘t discern render props from callbacks — this could lead to nasty subtle bugs.
  4. In the example you posted above (no idea if that’s solved on master, didn’t try it out) there are some very bad bugs introduced by your intermediate components optimizations:
    1. Have a look at the history prop: if it changes in a later re-render the variable _anonymousFnComponent2 will be already defined and will still point to the old history value. This is even more apparent with id, value in the array map: they will all render only the first item!
    2. You therefore need to use a specialized form of lambda lifting, transforming all free variables into explicit props (lambda lifting would transform them to arguments instead of props). Then you can re-use the same component between component instances and re-renders.
    3. Obviously you can’t do this trick if there are some let variables that the user re-assigns to during render. You need to identify those cases and bail out, probably with a warning.
    4. I would place a React.memo around those components, just in case :)
  5. Again, aggressively turning all elements to intermediate components is not a good idea IMHO. Could be a nice Babel Macro by itself, that let the user explictly mark his/her intention.
jedwards1211 commented 5 years ago

@yuchi I had a comment on another issue about how in my mind the ideal, pie-in-the-sky React compiler would be able to completely compile away virtual DOM diffing where possible...:smile: Can't find that issue at the moment

DAB0mB commented 5 years ago

@yuchi I'm just expecting to have a solution that works right out of the box. I don't even wanna think about it. If a machine can do it better than me, why should I even bother? If there would be enough awareness for this plug-in and activity from the community, there would be almost non to no bugs. Of course there are bugs in what I created, but it's only a proof of concept. About using let variable - you definitely can't use useMemo(), my plug-in handles that pretty nicely, Everything is solvable with a smart enough plug-in.