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

@uppy/react: add useUppyState #4711

Merged
merged 18 commits into from
Oct 24, 2023
Merged

@uppy/react: add useUppyState #4711

merged 18 commits into from
Oct 24, 2023

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Sep 28, 2023

Closes #4698
Closes #4680
Closes #4725

Problems

  1. If someone wants to use Uppy without one of our UI's, they have to recreate Uppy's entire state themselves. Meaning, listening to almost all events and then calling to useState hooks. Quite painful.
  2. If you are using dashboard, people still want to put Uppy inside useState because of the React mindset. Ending up with a fragile series of useState and useEffect.
  3. We say in the docs to always put Uppy outside of the component. This works okay for simple cases. But If you want to render multiple Uppy UIs, you do have to put Uppy in useState, even though we said before you shouldn't.

This PR focusses on one but outlines all problems to see how they relate to each other. Normally it's better to only talk about one thing in a PR, but I thought I proof of concept would help the discussion and it's good to talk about how the problems and solutions relate to each other. So feel free to provide feedback on all three.

Solution to 1 (this PR)

A new hook useUppyState which allows you to select a part of Uppy's state and the component will only re-render based on that state. This is mostly useful if you want to create a custom UI or if you use something like Dashboard but you just want to do something extra on the page somewhere based on Uppy state.

import Uppy from '@uppy/core'
import { Dashboard, useUppyState } from '@uppy/react'

export default function App () {
  // see next section why this is in useState
  const [uppy] = useState(() => new Uppy())
  const files = useUppyState(uppy, (state) => state.files)
  // We can also get specific plugin state
  const metaFields = useUppyState(uppy, (state) => state.plugins?.dashboard?.metaFields)

  return (
    <Dashboard id="dashboard" uppy={uppy} />
  )
}

Solution to 2 and 3 (future docs PR)

If you pass a function to useState, React will only call it during initialization. When re-rendering, the @uppy/default-store state remains in the background so uploads will continue. When the component is unmounted, its state (and uploads) will be reset.

Component based thinking means that you isolate state with your component. People writing React want to do this with Uppy. In fact, if you want to render an unknown amount of Uppy components, this is the only way.

Example: I want Uppy state to remain during re-renders but not during page change (unmount)

import React, { useState } from 'react'
import Uppy from '@uppy/core'
import { Dashboard } from '@uppy/react'

export default function App () {
  // important: passing an initialiser function to prevent the state from recreating!
  const [uppy] = useState(() => new Uppy())
  const [show, setShow] = useState(false)

  return (
    <>
      {/* toggle button */}
      {show && <Dashboard id="dashboard" uppy={uppy} />}
    </>
  )
}

Example: I want Uppy state to also remain during page changes

It's a common use case from customers, they want they're users to able to use their app (switch pages) while the upload continues. This is not something we can easily provide in a hook or store, unless we want to store everything in localStorage (not a good idea). However using React patterns this is possible: lifting state up.

import React, { useState, useEffect } from 'react'
import Uppy from '@uppy/core'
import { Dashboard, useUppyState } from '@uppy/react'

function Page1 () {
 // ...
}

function Page2 ({ uppy }) {
  const totalProgress = useUppyState(uppy, (state) => state.totalProgress)  
  return (
    <>
       <p>{totalProgress}</p>
      <Dashboard id="dashboard" uppy={uppy} />
    </>
  )
}

export default function App () {
  // keeping the uppy instance alive above the pages the user can switch during uploading
  const [uppy] = useState(() => new Uppy())

  return (
    <>
      <Page1 />
      <Page2 uppy={uppy} />
    </>
  )
}

@Murderlon Murderlon added the React React components or other React integration issues label Sep 28, 2023
@Murderlon Murderlon self-assigned this Sep 28, 2023
@socket-security
Copy link

socket-security bot commented Sep 28, 2023

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@types/react 18.2.28...18.2.23 None +0/-0 365 kB types

examples/react-native-expo/package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Murderlon Murderlon marked this pull request as draft September 28, 2023 12:15
@Murderlon Murderlon marked this pull request as ready for review September 28, 2023 12:55
@nickrttn
Copy link
Contributor

nickrttn commented Oct 2, 2023

As for the future work planned: what do you think about offering an UppyProvider that instantiates Uppy for users and manages stuff. That might make it easier for React users to reason about where Uppy is created, at the app, page or component level? That would also allow you to get rid of the first uppy argument to useUppyStateSelector as you could get it from context. I like the pattern used here: https://github.com/pmndrs/zustand/blob/main/docs/guides/initialize-state-with-props.md

@Murderlon
Copy link
Member Author

Murderlon commented Oct 2, 2023

As for the future work planned: what do you think about offering an UppyProvider that instantiates Uppy for users and manages stuff

I thought about it but refrained from doing so:

Context is very tempting to use! However, this also means it’s too easy to overuse it. Just because you need to pass some props several levels deep doesn’t mean you should put that information into contextReact docs

Their use cases for adding context also don't align with what Uppy is used for. Realistically, there is only one page where people upload in most apps and it would be overkill to provide a context for this in my opinion. I'd rather keep it simple and explicit for now. Perhaps reconsidering when we see people needing this more.

Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

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

simple and elegant!

lgtm except for comments by others

@Murderlon Murderlon changed the title @uppy/react: add useUppyStateSelector @uppy/react: add useUppyState Oct 2, 2023
* main:
  Bucket fn also remote files (#4693)
  fixup! Added Companion OAuth Key type (#4668)
  Added Companion OAuth Key type (#4668)
  meta: check for formatting in CI (#4714)
  meta: bump get-func-name from 2.0.0 to 2.0.2 (#4709)
  meta: run Prettier on existing files (#4713)
  Release: uppy@3.17.0 (#4716)
  meta: add Prettier (#4707)
  @uppy/aws-s3-multipart: retry signature request (#4691)
@aduh95
Copy link
Member

aduh95 commented Oct 2, 2023

Maybe a noob question: why does useUppyState return a simple value when React's useState returns an iterator? I guess that's because Uppy state is immutable, but shouldn't we try to be consistent nonetheless? Or maybe that's a usual pattern in React ecosystem

@Murderlon
Copy link
Member Author

Murderlon commented Oct 2, 2023

useState is for state you control, hence it's [value, setValue]. useSyncExternalStore is for state you do not control outside of React. You don't (and can't) do the setting of state and so we only return the value, based on your selector (chosen subset of state).

@Murderlon
Copy link
Member Author

@aduh95 still getting the same error as before when running yarn e2e:client and navigating to the React page 🤔

aduh95 and others added 4 commits October 5, 2023 21:40
* main:
  @uppy/vue: export FileInput (#4736)
  examples: update `server.py` (#4732)
  @uppy/aws-s3-multipart: fix `uploadURL` when using `PUT` (#4701)
  @uppy/dashboard: auto discover and install plugins without target (#4343)
  e2e: upgrade Cypress (#4731)
  @uppy/core: mark the package as side-effect free (#4730)
  Bump postcss from 8.4.16 to 8.4.31 (#4723)
  meta: test with the latest versions of Node.js (#4729)
…into new-react-hook

* 'new-react-hook' of https://github.com/transloadit/uppy:
  Revert unrelated change
  save react at the top level to workaround
@Murderlon
Copy link
Member Author

@aduh95 still getting the same error as before when running yarn e2e:client and navigating to the React page 🤔

This is now resolved 👍

@Murderlon Murderlon merged commit 12e08ad into main Oct 24, 2023
18 checks passed
@Murderlon Murderlon deleted the new-react-hook branch October 24, 2023 06:58
Murderlon added a commit that referenced this pull request Nov 1, 2023
* main:
  More image editor improvements (#4676)
  @uppy/react: add useUppyState (#4711)
  Release: uppy@3.18.1 (#4760)
  Bump jsonwebtoken from 8.5.1 to 9.0.0 in /packages/@uppy/companion (#4751)
  Bump react-devtools-core from 4.25.0 to 4.28.4 (#4756)
  Bump webpack from 5.74.0 to 5.88.2 (#4740)
  Bump @babel/traverse from 7.22.5 to 7.23.2 (#4739)
  @uppy/core: fix `sideEffects` declaration (#4759)
  Release: uppy@3.18.0 (#4754)
  @uppy/aws-s3-multipart: fix `TypeError` (#4748)
  Bump tough-cookie from 4.1.2 to 4.1.3 (#4750)
  example: simplify code by using built-in `throwIfAborted` (#4749)
  @uppy/aws-s3-multipart: pass `signal` as separate arg for backward compat (#4746)
  meta: fix TS integration (#4741)
@Jokcy
Copy link
Contributor

Jokcy commented Nov 5, 2023

@Murderlon I just notice that useSyncExternalStore should not be used like this, the getSnapshot should return be uppy.getState, it is how React know if the selector need to be be run again. If we make getSnapshot based on selector then if the selector alway return a new object, it will lead to infinite loop of component render.

For example useUppyState(uppy, (s) => Object.values(s.files)), even though s.files not changed, the Object.values(s.files) is always a new object.

There is another api from react use-sync-external-store package, which is useSyncExternalStoreWithSelector, we should use it instead to perform update on the selector function. I will make a PR for this.

BTW, it this api released? I tried to import from @uppy/react 3.1.4 but no this api found.

@github-actions github-actions bot mentioned this pull request Nov 8, 2023
github-actions bot added a commit that referenced this pull request Nov 8, 2023
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.5.0 | @uppy/provider-views   |   3.7.0 |
| @uppy/aws-s3-multipart |   3.9.0 | @uppy/react            |   3.2.0 |
| @uppy/companion        |  4.11.0 | @uppy/transloadit      |   3.4.0 |
| @uppy/companion-client |   3.6.0 | @uppy/tus              |   3.4.0 |
| @uppy/core             |   3.7.0 | @uppy/url              |   3.4.0 |
| @uppy/dashboard        |   3.7.0 | @uppy/utils            |   5.6.0 |
| @uppy/image-editor     |   2.3.0 | @uppy/xhr-upload       |   3.5.0 |
| @uppy/locales          |   3.4.0 | uppy                   |  3.19.0 |

- @uppy/dashboard: Remove uppy-Dashboard-isFixed when uppy.close() is invoked (Artur Paikin / #4775)
- @uppy/core,@uppy/dashboard: don't cancel all files when clicking "done" (Mikael Finstad / #4771)
- @uppy/utils: refactor to TS (Antoine du Hamel / #4699)
- @uppy/locales: locales: add ca_ES (ordago / #4772)
- @uppy/companion: Companion+client stability fixes, error handling and retry (Mikael Finstad / #4734)
- @uppy/companion: add getBucket metadata argument (Mikael Finstad / #4770)
- @uppy/core: simplify types with class generic (JokcyLou / #4761)
- @uppy/image-editor: More image editor improvements (Evgenia Karunus / #4676)
- @uppy/react: add useUppyState (Merlijn Vos / #4711)
@Murderlon
Copy link
Member Author

Thanks for the update. I can't find anything about useSyncExternalStoreWithSelector? It's not the in types I have locally nor is it on the react.dev website. The problem you describe is a common React problem though, whenever you put arrays in a dependency array. Regardless it should at least be documented.

@Jokcy
Copy link
Contributor

Jokcy commented Nov 11, 2023

@Murderlon react-redux use this hook, you can see the code here https://github.com/reduxjs/react-redux/blob/854f3e1687382925952712399f93c0e7d271cdbb/src/utils/useSyncExternalStore.ts#L2C94-L2C94 . And this hook is published by react offical repo, https://github.com/facebook/react/blob/main/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js.

The problem you describe is a common React problem though

Yes if we implement this way, but the recommend usage of useSyncExternalStore is to provide a stable getSnapshot function, normally it is the getState method of store. Use this to detect if store.state changed and then call selector.

arturi added a commit that referenced this pull request Nov 11, 2023
arturi added a commit that referenced this pull request Nov 12, 2023
@github-actions github-actions bot mentioned this pull request Nov 12, 2023
github-actions bot added a commit that referenced this pull request Nov 12, 2023
| Package            | Version | Package            | Version |
| ------------------ | ------- | ------------------ | ------- |
| @uppy/core         |   3.7.1 | @uppy/react-native |   0.5.2 |
| @uppy/dashboard    |   3.7.1 | uppy               |  3.19.1 |
| @uppy/react        |   3.2.1 |                    |         |

- @uppy/react: Revert "@uppy/react: add useUppyState (#4711)" (Artur Paikin / #4789)
- @uppy/dashboard: fix(@uppy/dashboard): fix wrong option type in index.d.ts (dzcpy / #4788)
- meta: fix build of TypeScript plugins (Antoine du Hamel / #4784)
- @uppy/core,@uppy/dashboard,@uppy/react-native: Update Uppy's blue color to meet WCAG contrast requirements (Alexander Zaytsev / #4777)
- meta: fix JS2TS script (Antoine du Hamel / #4778)
Murderlon added a commit that referenced this pull request Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React React components or other React integration issues
Projects
None yet
5 participants