fix(react): avoid unnecessary re-render in useIsServer

hrsh7th commented 1 month ago

The current implementation of useIsServer causes the render to happen twice.

This PR reduces re-render by using useSyncExternalStore way.

Closes #2965

vercel[bot] commented 1 month ago

@hrsh7th is attempting to deploy a commit to the Chakra UI Team on Vercel.

A member of the Team first needs to authorize it.

cschroeter commented 1 month ago

@hrsh7th

Thanks your for your PR. Is there any chance to provide some code so we can understand the difference between those two approaches?

From what we can tell, is that the useState is still updated using an effect so really would love to see some "proof" that it works as intended.

Cheers

hrsh7th commented 1 month ago

Thank you for your comment.

The following test passed on my end. So the new implementation seems to reduce the number of renderings in the case where container is not specified.

However, the test code is ugly and I am not sure if it will be acceptable. Should I also push the following test code?

  it('should render twice if container was specified', async () => {
    const RenderCount = () => {
      RenderCount.count++
      return <div>{RenderCount.count}</div>
    }
    RenderCount.count = 0

    const Test = () => {
      const ref = useRef<HTMLDivElement>(null)
      return (
        <div>
          <Portal container={ref}>
            <RenderCount />
          </Portal>
          <div ref={ref} />
        </div>
      )
    }
    const view = render(<Test />)
    await waitFor(() => expect(screen.getByText('2')).toBeInTheDocument())
    expect(view.baseElement).toMatchInlineSnapshot(`
      <body>
        <div>
          <div>
            <div>
              <div>
                2
              </div>
            </div>
          </div>
        </div>
      </body>
    `)
  })

  it('should render once if container was not specified', async () => {
    const RenderCount = () => {
      RenderCount.count++
      return <div>{RenderCount.count}</div>
    }
    RenderCount.count = 0
    const Test = () => {
      return (
        <div>
          <Portal>
            <RenderCount />
          </Portal>
        </div>
      )
    }
    const view = render(<Test />)
    await waitFor(() => expect(screen.getByText('2')).toBeInTheDocument(), {
      timeout: 500,
    }).catch(() => {})
    expect(view.baseElement).toMatchInlineSnapshot(`
      <body>
        <div>
          <div />
        </div>
        <div>
          1
        </div>
      </body>
    `)
  })

pkg-pr-new[bot] commented 1 month ago

Open in Stackblitz

@ark-ui/anatomy

``` bun add https://pkg.pr.new/chakra-ui/ark/@ark-ui/anatomy@2970 ```

@ark-ui/react

``` bun add https://pkg.pr.new/chakra-ui/ark/@ark-ui/react@2970 ```

@ark-ui/solid

``` bun add https://pkg.pr.new/chakra-ui/ark/@ark-ui/solid@2970 ```

@ark-ui/vue

``` bun add https://pkg.pr.new/chakra-ui/ark/@ark-ui/vue@2970 ```

@ark-ui/svelte

``` bun add https://pkg.pr.new/chakra-ui/ark/@ark-ui/svelte@2970 ```

commit: d2897c8

chakra-ui / ark

fix(react): avoid unnecessary re-render in useIsServer #2970