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

fix: improve feat/transitions type #410

Merged
merged 23 commits into from
Nov 23, 2023

Conversation

Talent30
Copy link
Contributor

@Talent30 Talent30 commented Nov 22, 2023

franky47 and others added 3 commits November 17, 2023 13:01
This lets non-shallow updates be notified of server rendering
loading status, using an external `React.useTransition` hook.
Copy link

vercel bot commented Nov 22, 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 0:21am

@Talent30 Talent30 changed the title Feat/transitions type fix: improve feat/transitions type Nov 22, 2023
@Talent30 Talent30 mentioned this pull request Nov 22, 2023
4 tasks
Copy link
Member

@franky47 franky47 left a comment

Choose a reason for hiding this comment

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

That's a very interesting approach!

suggestion: We could use better descriptive names for generic types than B and S (S stands for Shallow I guess, but is B for Boolean?)

issue: The end-to-end test bench fails to compile (run with pnpm test):

e2e:build: Failed to compile.
e2e:build: 
e2e:build: ./src/pages/pages/useQueryState/index.tsx:40:43
e2e:build: Type error: Type 'false' is not assignable to type 'undefined'.
e2e:build: 
e2e:build:   38 |         <button
e2e:build:   39 |           id="string_set_a"
e2e:build: > 40 |           onClick={() => setString('a', { shallow: false })}
e2e:build:      |                                           ^
e2e:build:   41 |         >
e2e:build:   42 |           Set A
e2e:build:   43 |         </button>

@Talent30
Copy link
Contributor Author

Talent30 commented Nov 22, 2023

That's a very interesting approach!

suggestion: We could use better descriptive names for generic types than B and S (S stands for Shallow I guess, but is B for Boolean?)

issue: The end-to-end test bench fails to compile (run with pnpm test):

e2e:build: Failed to compile.
e2e:build: 
e2e:build: ./src/pages/pages/useQueryState/index.tsx:40:43
e2e:build: Type error: Type 'false' is not assignable to type 'undefined'.
e2e:build: 
e2e:build:   38 |         <button
e2e:build:   39 |           id="string_set_a"
e2e:build: > 40 |           onClick={() => setString('a', { shallow: false })}
e2e:build:      |                                           ^
e2e:build:   41 |         >
e2e:build:   42 |           Set A
e2e:build:   43 |         </button>

Any way to get around it in TS? Sorry my brain is not functioning...
Update: build passed, back to bed...

function Test() {
  const [state, setName] = useQueryState('name')
  const [isLoading, startTransition] = React.useTransition()

  return (
    <>
      {/* failed Type '{}' is not assignable to type 'boolean | undefined'. */}
      <button
        id="string_set_a"
        onClick={() =>
          setName('a', {
            shallow: {}
          })
        }
      >
        Set A
      </button>
      {/* failed Type 'TransitionStartFunction' is not assignable to type 'undefined'.ts(2322)
       */}
      <button
        id="string_set_a"
        onClick={() =>
          setName('a', { shallow: true, startTransition: startTransition })
        }
      >
        Set A
      </button>
      <button
        id="string_set_a"
        onClick={() =>
          setName('a', { shallow: false, startTransition: startTransition })
        }
      >
        Set A
      </button>
      <button
        id="string_set_a"
        onClick={() => setName('a', { shallow: false })}
      >
        Set A
      </button>
      <button
        id="string_set_a"
        onClick={() => setName('a', { startTransition: startTransition })}
      >
        Set A
      </button>
    </>
  )
}

@Talent30
Copy link
Contributor Author

Talent30 commented Nov 23, 2023

I don't think it is going to be feasible.

Update:
This is my initial approach.

Typescript is not omnipotent; you must admit the unknowability of shallow option to Typescript before processing it through type exclusion logic. I think we can’t pass all the tests without introducing unknown because typescript doesn't check if boolean is true or false.

Update:
I am wrong, you can do it without unknown but you need to and extends boolean everywhere. Still don't think put it into the generics definition would work

Update:
I am wrong, forgot to build before running the test...

@Talent30
Copy link
Contributor Author

@franky47 This is the best I can do. Please approve this PR if you are happy with it.

Copy link
Member

@franky47 franky47 left a comment

Choose a reason for hiding this comment

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

praise: Great job! 👏

There are many corner cases where we could end up, like chaining withOptions, or overriding hook-level options with call-level ones, but I realised that the type for the Options object still make sense to be boolean | undefined and React.TransitionStartFunction | undefined, as the setting of shallow: false when passing a startTransition is not done when defining options, but when they get merged when applied to the URL queue.

@franky47 franky47 merged commit 885317c into 47ng:feat/transitions Nov 23, 2023
2 checks passed
franky47 added a commit that referenced this pull request Nov 23, 2023
* 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 added a commit that referenced this pull request Nov 23, 2023
* 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 added a commit that referenced this pull request Nov 23, 2023
* feat: Add support for transitions

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

* test: Add e2e test for transitions

* fix: improve the Option type for transitions (#410)

* 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]>

* doc: Add transitions docs

---------

Co-authored-by: Jon Sun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants