Test effects

[mindplay-dk]

Alright, here's a failing test that should demonstrate the problem we discussed in #79 .

As you can see, we're good on useEffect(f, [x]) as well as useEffect(f, []). 👍

The problem is with useEffect(f) which seems to trigger some effects and cleanups twice -

test('useEffect(f) should run every time', async () => {
  let effects = []
  let effectNum = 1

  const effect = () => {
    const currentEffectNum = effectNum++

    effects.push(`effect ${currentEffectNum}`)

    return () => {
      effects.push(`cleanUp ${currentEffectNum}`)
    }
  }

  const Component = () => {
    effects = []

    useEffect(effect)

    return <div>foo</div>
  }

  await testUpdates([
    {
      content: <Component/>,
      test: () => {
        expect(effects).toEqual(["effect 1"])
      }
    },
    {
      content: <Component/>,
      test: () => {
        expect(effects).toEqual(["cleanUp 1", "effect 2"]) 👍
      }
    },
    {
      content: <div>removed</div>,
      test: () => {
        expect(effects).toEqual(["cleanUp 2"]) 👎 /* LINE 262 */
      }
    }
  ])
})

As you will see when running the test:

    - Expected
    + Received
    
      Array [
    +   "cleanUp 1",
    +   "effect 2",
        "cleanUp 2",
      ]
        at Object.toEqual [as test] (/mnt/c/workspace/fre/test/render.test.jsx:262:25)

So, at removal, cleanUp 1 and effect 2 are currently both being run a second time!

Note that I did already merge your last commit which didn't fix it.

Hopefully this will help you fix it 😊

[yisar]

I think this is another bug, because I just fixed #79

[yisar]

Hey, I don't quite understand the example of this case. Maybe like this?

function App () {
  let effects = []
  let effectNum = 1
  const [state, setState] = useState(true)
  useEffect(() => {
    const currentEffectNum = effectNum++

    effects.push(`effect ${currentEffectNum}`)
    console.log(effects) // remove : ["effect 1", "cleanUp 1", "effect 2"]

    return () => {
      effects.push(`cleanUp ${currentEffectNum}`)
    }
  })
  return (
    <div>
      {state && <div>111</div>}
      <button onClick={() => setState(false)}>remove</button>
    </div>
  )
}

[mindplay-dk]

From the React guide, "effects with cleanup":

When exactly does React clean up an effect? React performs the cleanup when the component unmounts. However, as we learned earlier, effects run for every render and not just once. This is why React also cleans up effects from the previous render before running the effects next time.

With useEffect(f) (without the deps argument) the behavior is to clean up the last effect (if any) and then run the new effect - every time.

So the test runs in 3 steps to demonstrate this behavior.

Step 1: Component gets created - the effect gets triggered:

    {
      content: <Component/>,
      test: () => {
        expect(effects).toEqual(["effect 1"])
      }
    },

Step 2: Component gets updated - the first effect gets cleaned up, and then the second effect gets triggered:

    {
      content: <Component/>,
      test: () => {
        expect(effects).toEqual(["cleanUp 1", "effect 2"])
      }
    },

Step 3: Component is removed - the second effect gets cleaned up:

    {
      content: <div>removed</div>,
      test: () => {
        expect(effects).toEqual(["cleanUp 2"])
      }
    }

[yisar]

Can you give me a use case? The use case I try here is the same as the result of react, so I think the implementation of fre should be correct now

[mindplay-dk]

Here's a demo that prints the effects and clean-ups for each operation:

https://codesandbox.io/s/fre-demo-fgymz

You can switch it between imports from react or preact (which work the same) and a copy of fre that I've just built from master.

If I repeat an exact sequence of clicks with react, preact and fre, and copy the console output from each, react and preact are identical - here's a diff against the output from fre:

That's react on the left, and fre on the right.

As you can see, react and preact do not repeat the same effects or clean-ups, but fre does.

You need to revert this change - the test should be correct, and should pass if your output is like the one on the left.

[yisar]

Thank you very much! I will debug and fix it🙏🙏🙏

[yisar]

I fixed this bug. Now it looks the same as react, but I still can't run through the test case. Can you find the reason?

[mindplay-dk]

Looks like you still need to roll back the change you made to the test?

[yisar]

The test is still not necessarily correct. Can you change the example for differentiation? I fixed the last example.
Don't create tests without use cases, it's not convincing

[yisar]

I am very concerned about this bug. Our purpose is to find out the difference between react and fre, not the test is right or not.
Before finding a persuasive use case, the test is uncertain, because we can't run the react test, there is only the react use case.

[mindplay-dk]

But I don't see any new commits to anything other than files in the demo folder?

Maybe you didn't push all your changes? (or are they on a branch?)

I just fetched master, ran a build and updated this sandbox - compared output with diff again, and the output is exactly the same as before.

Here they are aligned, side-by-side, for easier comparison:

Red lines show effects getting fired twice.

Everything else looks correct, just that sometimes the same effects and clean-up get triggered twice?

[yisar]

I commit again, but from here, Remove seems to be OK, Increment is another bug.

[mindplay-dk]

Reverse also looks fine.

It looks like effects are triggered twice only during updates involving the useState setter?

[yisar]

It seems like this. We could keep up with react as much as possible.

[yisar]

The above is a good summary, I think usecomponent will works well, which is amazing!

[mindplay-dk]

The above is a good summary, I think useComponent will works well, which is amazing!

Thanks, but it's just a toy for now - a lot more work is required to support all of the life-cycle methods, etc... I think it's possible, though I'm not 100% sure yet.

What's interesting, is this would be the opposite of what Preact does: they have the Component API built into the core, and hooks are an optional add-on - Fre would instead have hooks built into the core, with class-based components as an optional add-on. 👀

Another really interesting thing about this, is that, with React/Preact, you have to choose either class-based components or hooks... But with this approach, Fre would actually allow mixing class-based components and hooks - you'd be able to call hooks from the render-method of a Component, which is interesting; for example, this would allow you to gradually move features from a large class into hooks, rather than having to refactor everything all at once.

[yisar]

But with this approach, Fre would actually allow mixing class-based components and hooks - you'd be able to call hooks from the render-method of a Component.

Ah~as you said, This is really amazing.