React: Bug: Overwriting console.log during rendering

[Jack-Works]

React version: latest

Steps To Reproduce

function App() {
  return <h1 onClick={console.log}>h1</h1>;
  // console.log is replaced by "disableLog" so I can't log any event.
  return <h1 onClick={x => console.log(x)}>h1</h1>;
  // have to do this
}

Previous discussion at https://github.com/facebook/react/pull/15894#issuecomment-668542243

[gaearon]

I think there's some misunderstandings here. console.log is definitely not disabled during every render. It's only disabled during a pass that gets thrown away.

I would expect

function App() {
  return <h1 onClick={console.log}>h1</h1>;
}

to work. If it doesn't, maybe it means we're using the result of the wrong render?

[Jack-Works]

Hmm In my working on PR #15894 I found this. Let me try it in another version of React.

[gaearon]

I mean, it's possible that there is a bug. We do disable logging during one of the render passes on master. I would just expect that to only happen for the pass whose result gets ignored.

[gaearon]

It does look like we're using the second result.

https://github.com/facebook/react/blob/e9721e14e4b8776c107afa3cdd7c6d664fe20c24/packages/react-reconciler/src/ReactFiberBeginWork.old.js#L738-L763

Maybe we should just ignore it? Not sure if there are other considerations.

Alternatively, maybe we should silence the first render?

[bvaughn]

Alternatively, maybe we should silence the first render?

I think the danger here would be with memoization (outside of React). If you're using e.g. lodash.memoize, you may close over and memoize a disabled value. Then again, I guess the same thing could happen with the second render (if you happened to be using e.g. console.log as a dependency)

[Jack-Works]

Found this behavior (`disableLog` passed to `onClick`) in 0.0.0-experimental-3d0895557.

Again, my view:

Actually, duplicate logs will warn the developer about they shouldn't do side effect things out of useEffect. It actually helps developers to figure out what is "double render"

Duplicated logs are the side effects that very simple to discover and learn about.

If we don't teach developers not to write side effects in the component (by duplicating the log, the easiest way to know about), they will write side effects that not so easy to find and introduce bug in future.

[gaearon]

@Jack-Works We understand the argument well, there is no need to repeat it. We've had debates about this too and we are giving this approach a try. You can imagine we also felt conflicted about it but so far it seems promising in practice. (Aside from the issue in this thread, which is definitely something worth fixing IMO.)

Again, the part about breaking event handlers is not intentional.

[zhangenming]

var outer= console.log
function Test1() {
  var inner= console.log
  console.log(inner === outer) //true
  return (
    <>
      <h1 onClick={outer}>h1</h1> // OK
      <h1 onClick={inner}>h1</h1> // bad
    </>
  )
}

Confused...

[Jack-Works]

See? People are confusing with this change. 🤔

[gaearon]

@Jack-Works I appreciate your enthusiasm about this issue but it doesn't help if you restate what another person has just commented. This creates noise in the notifications for everyone. Please keep the comments substantial.

It is an intentional tradeoff that we're trading one sort of confusion for another sort of confusion here. So this is not new data.

[zhangenming]

var outer= console.log
function Test1() {
  var inner= console.log
  console.log(inner === outer) //true
  return (
    <>
      <h1 onClick={outer}>h1</h1> // OK
      <h1 onClick={inner}>h1</h1> // bad
    </>
  )
}

Confused...

Now I know what's going on,
We know that the render will be performed twice, and the second console.log will be disable
console.log(inner === outer) here inner we get from the first render
onClick={inner} but this inner we get from second