chore(tests): remove imperfectly working test added in #276

[lucasrabiec]

Fixed console error about React 17 -> 18 in basic.test.tsx

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2719b35:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration
Next.js	Configuration

[dai-shi]

That test was added in some PR to catch some bugs.

1. I'm not sure if the test works (catch bug) with createRoot.
2. We run tests in pre 18, so this doesn't pass CI.

Maybe we should just leave it? Or, would you like to investigate (1)?

[lucasrabiec]

Oh, I see. react-dom/client import broke pre 18 tests.
Here is what came to my mind:

it('re-renders with useLayoutEffect', async () => {
  const useBoundStore = create(() => ({ state: false }))

  function Component() {
    const { state } = useBoundStore()
    useLayoutEffect(() => {
      useBoundStore.setState({ state: true })
    }, [])
    return <>{`${state}`}</>
  }
  const container = document.createElement('div')

  await actAndAssertBasedOnReactVersion(container, <Component />, () => {
    expect(container.innerHTML).toBe('true')
  })
})

async function actAndAssertBasedOnReactVersion(
  container: Element | DocumentFragment,
  componentToRender: JSX.Element,
  expectCallback: () => void
) {
  let root: Root
  const isReactPost18 = parseInt(React.version) > 17

  if (isReactPost18) {
    // eslint-disable-next-line @typescript-eslint/no-var-requires
    const { createRoot } = require('react-dom/client')
    root = createRoot(container)
  }

  isReactPost18
    ? act(() => root.render(componentToRender))
    : ReactDOM.render(componentToRender, container)

  await waitFor(expectCallback)

  isReactPost18
    ? act(() => root.unmount())
    : ReactDOM.unmountComponentAtNode(container)
}

I think it should solve it for all tests but I am not sure whether it is the best approach. Also needs var-require inside the test 😕
Thanks to this solution we are sure the test renders the component via the appropriate version but from the other side, it's not the cleanest approach IMO.
What do you think?

[dai-shi]

Well, I think we could just skip the test for react 18...

Would you please have a look at #280 and see why we have this test?
Maybe it's no longer so valid?

Anyway, I think it's better to hide warning instead of using createRoot.

const consoleError = console.error
beforeEach(() => {
  console.error = jest.fn((message) => {
    if (
      message.startsWith('Warning: ReactDOM.render is no longer supported')
    ) {
      return
    }
    consoleError(message)
  })
})
afterEach(() => {
  console.error = consoleError
})

[lucasrabiec]

Hmm. It looks like the test is useless. In pre 3.3.1 was a problem with useEffect (https://codesandbox.io/s/zen-goldstine-1571u4 - flickering screen red/green). @leobastiani changed it to useIsoLayoutEffect. I don't see this code in version 4 anymore but it looks like the bug reappeared. Anyway, the test doesn't catch this bug, I removed it.

[dai-shi]

but it looks like the bug reappeared.

What does this mean?

I think we still use useLayoutEffect somewhere in deep (not our code),
and the test still makes sense. Let's keep the test until we drop old React versions.

[lucasrabiec]

What does this mean?

Look at the linked codesandbox. The background blinks to red only on versions pre 3.3.1 and post 4-alpha. Eg. switch zustand's version to 3.1.4 (red blink), then 3.3.2 (no blink) and then 4.0 (red blink). I think it is the same bug that was reported in #276

Edit: IMO we can close this PR without removing this test and without disabling console.error. I agree that it is reasonable to have both.

[dai-shi]

Oh, I see. So, the #276 behavior is reproduced with zustand v4.0.0 and react 17, but the test doesn't catch it. Do you happen to know how to fix this??

This is probably an intentional behavior or a bug in use-sync-external-store/shim. We could report an issue in the react repo.

I'm actually fine to remove the test if it's an intentional behavior or in any case.

---

A side note: in your codesandbox, you use a boolean value as a store. #1144 might be of your interest.

[lucasrabiec]

Do you happen to know how to fix this??

Unfortunately not. Maybe @leobastiani will know because he fixed it before. I will need to dig into it more, but I don't have time currently.

I'm actually fine to remove the test if it's an intentional behavior or in any case.

Ok, so I will not revert the last commit.

A side note: in your codesandbox, you use a boolean value as a store. #1144 might be of your interest.

It's not my code, I copied URL to it from #276 but GTK (+1)

[dai-shi]

Merging this because I think as follows:

• Since v4, the behavior this test is trying to check is broken (it's breaking change from v3)
• We don't know how to write such a test with createRoot
• Even if we know how to write a test, the fix should be in use-sync-external-store (so, we should file an issue there).