Timing of useEffect (error in render-test)

[mindplay-dk]

The fix in #86 created a problem with the timing of useEffect.

You wrote:

Before I implement the async useeffect, I must use setTimeout to ensure that I get the correct result after DOM operation.

To work around this regression, you introduced a call to setTimeout, which shouldn't be necessary.

An ordinary use-case for effects is in conjunction with refs - consider the following basic example, which currently doesn't work:

const Component = () => {
  const ref = useRef();

  useEffect(() => {
    console.log(ref.current); // ❌ undefined
    // ref.current.style.backgroundColor = "red"; 💣
  }, []);

  return <div ref={ref}>HELLO</div>;
};

Note that, due to #84, I had to comment out the style assignment in the example above - this is just so you can see ref.current showing as undefined on the console, which should be the element reference.

So the effect is firing too soon - before the DOM updates have been applied.

To demonstrate precisely how this should work, I created an example using all 3 kinds of effects to generate console output that precisely demonstrates the order in which effects and clean-up should be dispatched:

https://codesandbox.io/s/smoosh-silence-1unrp

To demonstrate the timing of effects and clean-up in relation to the DOM updates, the result of the test function is printed every time - this function tests the DOM to see if the element exists and has the expected state, meaning the expected change has been applied to the DOM. In other words, this tells us whether the effect (or clean-up) function is being called BEFORE update or AFTER update, and you can see this for every call on the console.

Here is a comparison of output between react, preact and fre:

During creation and updates, the timing of dispatch of effects and clean-up against DOM updates is currently the opposite of React. (During removal, it is correct.)

Also, the order is wrong, with new effects being dispatched before clean-up of previous effects - as well as clean-up from effects at creation being dispatched much later during removal instead of getting cleaned up during the next update as expected.

(I hope this is helpful... The approach of hands-on "tests" like this is very fragile, but I still don't know how to turn this into an automated test - which is definitely something we need to do to prevent any future regressions...)

[yisar]

I'm trying to implement it. In fact, I haven't figured it out completely yet. Give me some time🤯.

[yisar]

I tried to refactor useeffect, which now uses the requestAnimationFrame and executes before the next frame is repaint.
here

I don't know why the test didn't pass, but the use cases worked well

[mindplay-dk]

The tests aren't waiting for the next frame, so they'll execute too soon, I think?

I don't think you can simply delay cleanup by another frame this way - it might appear to work fine when you're testing by just clicking around in a simple UI, but when updates trigger effects etc. in a complex app, you might for example end up trying to run a cleanup function on elements that have already been removed by another update in the previous frame...

Clean up is part of the reconciliation process - it all needs to happen as one complete operation, before the scheduler can start another operation, I think?

[yisar]

For deleted elements, it cannot be executed later because it is equivalent to componentWillUnmout
For the updated element, the next frame is really early. It may executed util setTimeout 0
I think react is too complicated, so I simplify it, and also you can use it like this:

useEffect(()=>{
  setTimeout(fn,0)
})

[mindplay-dk]

But doing this in user code just creates the same problem: whatever you're doing after that timeout, you can't be sure that the state of the DOM is still what you need/expect it to be - removed elements can cause errors, and so on.

It is complicated, but I'm not sure this can really be simplified.

[yisar]

Halo, I spent a long time debugging our test files today. Now, only the effects test runs incorrectly.

I find that this is not an implementation error of fre, nor is it because the execution time of requestAnimationFrame is too early, Maybe our testUpdates method is not correct.

For example, I derfer 20ms and console the effects here

setTimeout(() => {
    console.log(effects) // this is correct
}, 16)
expect(effects).toEqual(['effect'])

Then I try to change the code:

setTimeout(() => {
    expect(effects).toEqual(['effect'])
}, 16)

In theory it works, but it's wrong, it throws an error

I'm still not familiar with jest, but for sure, it's not caused by requestAnimationFrame.

In addition, can I close the issue about refs, except that all the tests of effects have been passed now.

[yisar]

@mindplay-dk Halo, I modified the test code, and now all tests are passed. Now let me explain why.

The tests aren't waiting for the next frame, so they'll execute too soon, I think?

Yes, you are right, as you say, I must defer it, so I add a nextTick method.

const nextTick = fn => {
  let start = Date.now()
  while(Date.now() < start + 16){}
}

Use it and wait for next frame, but how to get the results of useEffect?

In your code , there is a global effects array, which can collect effect from last time:

let effects = []
update 1 => []
update 2 => [effect1]
update 3 => [effect1,cleanUp1]

So this is a good summary. First of all, I delayed the execution, and each update get the effect from the last time.

Now our test runs well 💯 , Thank for your great job 👍 ！！

[yisar]

Wow! I found a better way to tests here

Maybe we also can do same util useLayout have been implemented.

[yisar]

I changed it again

useEffect(()=>{
 return ()=>{
    console.log('cleanup')
 }
},[])

cleanup will console only while the component will remove.

[mindplay-dk]

You seem to be changing the test conditions with no explanation? This test makes no sense anymore.

And what is this? This won't work - this will merely block the main thread for some time, then returns control to the call site.

You were hoping to give time back to the renderer, presumably? You can't do it this way - calls to this function will just block for a moment and then return control to the call site without giving any time back to the render scheduler.

You can't just change the test conditions whenever the tests fail - that's not how testing works. The point of testing is to satisfy a specification - it's not merely about showing how the code currently works. You can change the specification, of course, if your architecture/requirements have changed - but you shouldn't just change the test-conditions whenever they don't match what the library is doing.

I can't keep up with all these changes, and I'm fairly sure that the test-suite is more or less completely broken at this point - many of the test conditions are broken, and many of the tests aren't testing the updated state of the DOM at all, so the tests have very little value at this point.

I'd like to help, but you need to trust the tests (which I understand was impossible when the tests stopped working altogether because of the changes to scheduling and effects) and don't change them unless you're sure you understand what the test is for, how it works, or why your requirements changed. Ask me if you're unsure how something works, or add me as reviewer on PRs if you think the tests are wrong.

Now, if you want, I can try to go over all the changes to the test-suite again, and try to restore it - but as I've said in the past, we need to figure out a way to make this library testable at the core... We need a non-blocking way for testUpdates to know when the queue is empty and all the updates are complete - and we can't do it merely by waiting for some unspecified period of time, this has to be a predictable operation. (requestAnimationFrame, for example, will not work.)

[yisar]

@mindplay-dk
I'm sorry, because some of the changes are large and many tests are broken already. Starting tomorrow, I will carefully study the code of our tests and change the test ofs PR and explain why

Specifically, I haven't found a way to test the requestanimationframe. It seems that we can make the useeffect sync (same as useLayout) and then tests it.

[mindplay-dk]

Specifically, I haven't found a way to test the requestanimationframe. It seems that we can make the useeffect sync (same as useLayout) and then tests it.

I think this problem started after I pointed out the difference between useEffect and useLayoutEffect - as I recall, I pointed out that our current useEffect, being synchronous, probably worked more like useLayoutEffect works in React, and I think you went ahead and changed useEffect to make it work more like React?

Probably, what should have happened, is useEffect should have been renamed to useLayoutEffect, since it already worked that way - and a new useEffect hook should have been introduced instead?

Right now, we don't have anything that works like React's useLayoutEffect, which is what we needed in testUpdates - if we had a useLayoutEffect that worked like React's, we could have changed testUpdates to use that instead, and the tests should work the same.

For reference, note how useLayoutEffect in Preact uses synchronous render callbacks (which get flushed after browser paint) - whereas useEffect uses asynchronous pending effects (which get invoked immediately in the commit phase.)

[yisar]

we don't have anything that works like React's useLayoutEffect, which is what we needed in testUpdates

Yeap, I've changed useeffect now, it's closer to react, but maybe we need uselayout to service the tests.
So, I will add uselayout in a moment, and then we will modify the test again……

[mindplay-dk]

I see that you've also been working on the test since I pulled master and started working on it myself this morning, so I don't know how useful this will be, but...

mindplay-dk@ddf8695

Please see inline comments, and the comment at the bottom of the page, for various notes.

As you can see, I restored basically all the test-conditions to the way they were before you started changing them - all the tests except one are now passing (if you run them individually) so as you can see, the test conditions weren't wrong, it was the framework (testUpdates etc.) that wasn't working due to timing issues.

There are still serious timing issues, and I solved them with hacks for now. 😣

I don't know how to fix them.

Honestly, Jest is a nightmare to work with - pretty much the worst test experience I've ever had with any test-framework. 😐

Anyhow, read the comments on the commit page for more details...

[yisar]

This is very useful, until this afternoon, I have now completed the code changes

[mindplay-dk]

This is very useful, until this afternoon, I have now completed the code changes

I'm not sure what you mean.

Most of the test-conditions currently in master are still wrong.

[yisar]

I mean, your branch is useful~

The cause of useeffect (callback, [x]) test failure is that my code yesterday is incorrect. Please pull the latest code.

Maybe it's still a bug?

[mindplay-dk]

See #111

[yisar]

Based on the latest commit, all tests are passed and the build button turns green again. I think we can turn off several PR now.
#87 #97 #91 #62

For the above problems, we can selectively close them, because now the test cases are supported and tested.

[yisar]

After using useLayout instead of useEffect, all the tests passed, so I think this problem has been solved. I'll turn it off now and turn it on later when there is a problem.