Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for transitions #406

Merged
merged 5 commits into from
Nov 23, 2023
Merged

feat: Add support for transitions #406

merged 5 commits into from
Nov 23, 2023

Conversation

franky47
Copy link
Member

@franky47 franky47 commented Nov 17, 2023

This lets non-shallow updates be notified of server rendering loading status, using an external React.useTransition hook.

Example usage:

function ClientComponent() {
  // 1. Provide your own useTransition hook:
  const [isLoading, startTransition] = React.useTransition()
  const [query, setQuery] = useQueryState(
    'query',
    // 2. Pass the `startTransition` as an option:
    parseAsString().withOptions({ startTransition })
  )
  // 3. isLoading will be true while the server is re-rendering and streaming RSC payloads
  // when the query is updated via `setQuery`.
}

Tasks

  • Investigate dangling startTransition functions in unmounted components (see below)
  • Improve type safety around the interaction of the shallow and startTransition options. Done in fix: improve feat/transitions type #410, thanks @Talent30 !
  • Add end-to-end tests
  • Add documentation

Things that need to be ironed out:

What happens if we were to call a startTransition on an unmounted component? Because the URL update is deferred, a query state update may cause the calling component to be unmounted, and its startTransition reference left dangling in the queue. This is an issue with state updates, is it the same for transitions, and how can we mitigate it? -> This is no longer an issue in React 18 apparently: reactwg/react-18#82

Closes #402.

Copy link

vercel bot commented Nov 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-usequerystate ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 23, 2023 11:15pm

*
* Using this will set the `shallow` setting to `false` automatically.
*/
startTransition?: React.TransitionStartFunction
Copy link
Contributor

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was suggested by @Talent30 in #402 (comment), thanks for doing the heavy lifting for types!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second this

Copy link
Member Author

@franky47 franky47 Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is indeed possible to represent the Options object this way, it makes consuming it in a generic way impossible within the hooks, without resorting to // @ts-expect-error.

When we merge options declared at the hook level with options passed as arguments of the state updater function, we just run a simple merge, and let the update queue decide how to merge the result with the queue options. But this excluding union doesn't have any valid intersection ({ shallow?: boolean; startTransition?: ReactTransitionStartFunction}), so TypeScript complains.

I wondered about allowing startTransition in shallow mode, but the loading state remains false in that case, unsurprisingly. I'll have to see if I can find a valid use-case for using the raw startTransition function (without a loading observer), to mark a shallow URL update (and associated scroll) as interruptible by other UI events.

Edit: since we're not setting React state in the startTransition in shallow mode, it effectively makes it pointless.

In the mean time, I'll probably move on to merging this without the discriminated union type, and let the documentation explain the behaviour. What do you think @tordans @Talent30 ?

Copy link
Contributor

@Talent30 Talent30 Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a minimum of type safety can be ensured that would be great otherwise it shouldn't block this PR from moving on.

Update:
Not sure if you would be happy with this solution #410 (comment)

The solution can handle the following cases:

// pass
useQueryState('key', {
  shallow: false
})
// pass
useQueryState('key', {
  shallow: true
})
// pass
useQueryState('key', {
  startTransition: startTransition
})
// pass
useQueryState('key', {
  shallow: false,
  startTransition: startTransition
})
// failed No overload matches this call.
//  The last overload gave the following error.
//    Type 'TransitionStartFunction' is not assignable to type 'undefined'.
useQueryState('key', {
  shallow: true,
  startTransition: startTransition
})

@Talent30
Copy link
Contributor

For unmounted components I don't think there is a elegant solution at the moment.

I can only think of using a ref to keep track of the component's mounting status.

Will React throw an error if we call startTransition in an unmounted component?

@franky47
Copy link
Member Author

Will React throw an error if we call startTransition in an unmounted component?

No, it doesn't seem to. Neither does it throw an error if setting state on an unmounted component, is this something that's now legal in React? I must have missed the memo.

Here's my test bench:

'use client'

import { parseAsBoolean, useQueryState } from 'next-usequerystate'
import React from 'react'

const parser = parseAsBoolean.withDefault(true)

export function Client() {
  const [showBench, setShowBench] = useQueryState('showBench', parser)
  return (
    <>
      {showBench && <TransitionBench />}
      {!showBench && <button onClick={() => setShowBench(true)}>Reset</button>}
    </>
  )
}

function TransitionBench() {
  const [, startTransition] = React.useTransition()
  const [, setShowBench] = useQueryState('showBench', parser)
  const [state, setState] = React.useState('idle')
  console.log('render TransitionBench', state)

  React.useEffect(() => {
    console.log('Mounting TransitionBench')
    return () => {
      console.log('Unmounting TransitionBench')
    }
  }, [])

  return (
    <>
      <button
        onClick={() => {
          setTimeout(() => {
            startTransition(() => {
              console.log('In the transition body')
              // This should fail, but it doesn't
              setState('unmounted?')
            })
          }, 500)
          setShowBench(false)
        }}
      >
        Start Transition & Remove component
      </button>
      <p>{state}</p>
    </>
  )
}

@Talent30
Copy link
Contributor

Will React throw an error if we call startTransition in an unmounted component?

No, it doesn't seem to. Neither does it throw an error if setting state on an unmounted component, is this something that's now legal in React? I must have missed the memo.

Here's my test bench:

'use client'

import { parseAsBoolean, useQueryState } from 'next-usequerystate'
import React from 'react'

const parser = parseAsBoolean.withDefault(true)

export function Client() {
  const [showBench, setShowBench] = useQueryState('showBench', parser)
  return (
    <>
      {showBench && <TransitionBench />}
      {!showBench && <button onClick={() => setShowBench(true)}>Reset</button>}
    </>
  )
}

function TransitionBench() {
  const [, startTransition] = React.useTransition()
  const [, setShowBench] = useQueryState('showBench', parser)
  const [state, setState] = React.useState('idle')
  console.log('render TransitionBench', state)

  React.useEffect(() => {
    console.log('Mounting TransitionBench')
    return () => {
      console.log('Unmounting TransitionBench')
    }
  }, [])

  return (
    <>
      <button
        onClick={() => {
          setTimeout(() => {
            startTransition(() => {
              console.log('In the transition body')
              // This should fail, but it doesn't
              setState('unmounted?')
            })
          }, 500)
          setShowBench(false)
        }}
      >
        Start Transition & Remove component
      </button>
      <p>{state}</p>
    </>
  )
}

Very odd, in my past experience it should throw Can't perform a React state update on an unmounted component, is that because we put it under startTransition and somehow it handles it?

@franky47
Copy link
Member Author

franky47 commented Nov 17, 2023

@Talent30 no apparently it's now OK to do that in React 18: reactwg/react-18#82 (TIL).

In a memory management point of view, what we're doing here is safe: we're storing references of the startTransition function in a queue, that will get emptied when the URL gets updated.

When a component unmounts, the only reference left is the one in the queue. When the queue gets flushed, it won't do anything particular wrt/ transitions, but once the URL update function exits, the last startTransition reference gets cleared and it can get garbage-collected.

@Talent30
Copy link
Contributor

Talent30 commented Nov 17, 2023

A draft doc.

React Server Components offer a blend of server-side rendering and client-side interactivity. The use of the useTransition hook, combined with useQueryState, you can manage the loading state effectively when non-shallow updates are in progress.

Example Snippet

Server Component:

import { parseAsString } from 'next-usequerystate/parsers';

const queryParser = parseAsString.withDefault('');

export default async function ServerComponent({
  searchParams,
}) {
  const query = queryParser.parseServerSide(searchParams.query);
  const data = await api.getData({ searchParams: { query } });

  return (
    <ClientComponent data={data} />
  );
}

Client Component:

When you opt to invoke startTransition, it is important to recognise that the feature automatically turns off the shallow mode.

'use client';
import React from 'react';
import { useQueryState, parseAsString } from 'next-usequerystate';

function ClientComponent({ data }) {
  const [isLoading, startTransition] = React.useTransition();
  const [query, setQuery] = useQueryState('query', parseAsString().withOptions({ startTransition }));

  // Indicate loading state
  if (isLoading) return <div>Loading...</div>;

  // Normal rendering with data
  return <div>{/*...*/}</div>;
}

@franky47
Copy link
Member Author

franky47 commented Nov 21, 2023

Not sure if data streaming is the right word to use

@Talent30 Not really, I did a test where the page component would resolve after 500ms, and includes a server component under a Suspense boundary that resolves after 1000ms, and expected the page update to be streamed in first, then the child RSC to be streamed in last and resolve the transition loading state, but it all updates in one go after 1000ms. 🤷‍♂️

Thanks for your doc draft, it's a bit verbose though, the feature is simple enough to directly show a code example (and an interactive demo once the docs site is up).

@Talent30
Copy link
Contributor

Not sure if data streaming is the right word to use

@Talent30 Not really, I did a test where the page component would resolve after 500ms, and includes a server component under a Suspense boundary that resolves after 1000ms, and expected the page update to be streamed in first, then the child RSC to be streamed in last and resolve the transition loading state, but it all updates in one go after 1000ms. 🤷‍♂️

Thanks for your doc draft, it's a bit verbose though, the feature is simple enough to directly show a code example (and an interactive demo once the docs site is up).

I have shortened the doc

franky47 and others added 5 commits November 24, 2023 00:13
This lets non-shallow updates be notified of server rendering
loading status, using an external `React.useTransition` hook.
* feat: Add support for transitions

This lets non-shallow updates be notified of server rendering
loading status, using an external `React.useTransition` hook.

* fix: improve typing

* fix: better typing

* fix: remove unnecessary code

* chore: remove unnecessary changes

* fix: some edge cases when shallow is not a boolean type

* fix: remove as any

* fix: e2e build

* chore: better name for generic

* fix: parser type

* fix: better naming

* fix: better naming

* chore: better naming

* test: Add type tests for shallow/startTransition interaction

* fix: simplify type

* test: add a extra test case

* chore: extract type exclusion logic

* chore: simplify types

* chore: remove unnecessary generics

* chore: add test case for startTransition

* test: Add type tests

* chore: Simplify type & prettify

---------

Co-authored-by: Francois Best <[email protected]>
@franky47 franky47 merged commit 1b53ea9 into next Nov 23, 2023
12 checks passed
@franky47 franky47 deleted the feat/transitions branch November 23, 2023 23:16
Copy link

🎉 This PR is included in version 1.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting pending state of the router
3 participants