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

Export 'state-upadte' event on types #4698

Closed
2 tasks done
Jokcy opened this issue Sep 25, 2023 · 7 comments · Fixed by #4711
Closed
2 tasks done

Export 'state-upadte' event on types #4698

Jokcy opened this issue Sep 25, 2023 · 7 comments · Fixed by #4711
Labels

Comments

@Jokcy
Copy link
Contributor

Jokcy commented Sep 25, 2023

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

No response

Steps to reproduce

When use uppy.on('state-update') to listen for state-update event, we will get type error because state-update event did not exported in type declaration

Screenshot 2023-09-25 at 16 05 08

This event is very useful for frameworks like react, it should be exposed.

Expected behavior

Expose state-update event in uppy core declaration.

Actual behavior

No state-update event exposed in uppy core declaration.

@Jokcy Jokcy added the Bug label Sep 25, 2023
@Murderlon
Copy link
Member

At the moment this is meant as a internal only event, hence it's not documented. We may want to rethink this event in #3938 and #3248 before making it public.

@Jokcy
Copy link
Contributor Author

Jokcy commented Sep 25, 2023

@Murderlon So the default-store thing also not stable now? What if I create my own store instance and pass to Uppy constructor, and then I subscribe to the store instance, is this stable to use now? The doc of this part is also marked as TODO currently.

Uppy is very great, I'm currently working on my application based on it, if I can help with this store thing, fell free to let me know.

@Murderlon
Copy link
Member

Stores are stable! I'm realising now the docs on custom stores and our @uppy/redux-store got lost in the transition to the new docs website. Created an issue for it.

What are you trying to achieve with state-update though? I'm pretty sure it would cause too many rerenders in React than needed if you hook that up to useState.

@Jokcy
Copy link
Contributor Author

Jokcy commented Sep 26, 2023

I'm tring to implement a useSelector hook for uppy state, just like what redux did. I suppose the easily way to do this is by listening to the state-update event, so that user don't need to care about the Store implemetation. Someting like:

function useUppySelector(uppy, selector) {
    const [state, setState] = useState(() => selector(uppy.getState()));

    useEffect(() => {
        const handler = (prevState, nextState) => {
            setState(selector(nextState));
        };

        uppy.on(
            "state-update",
            handler,
        );
        setState(() => selector(uppy.getState()));

        return () => {
            uppy.off("state-update", handler);
        };
    }, [uppy, selector]);

    return state;
}

Because this hook state rely on part of uppy state, so I suppose it will not be too much useless rerender, but you are right, I will check it later.

And I can also do it based on store instance.

@Murderlon
Copy link
Member

Coincidentally, I'm looking into improving Uppy in React this week or next week. Was also thinking of some kind of useUppyStateSelector hook and a React specific store. I'm not sure how it will work yet.

Regarding your implementation, your selector doesn't select anything yet? Hence it will update on all state changes? Perhaps you can get away with it if you useMemo a part of the state you're going to use in your UI.

@Jokcy
Copy link
Contributor Author

Jokcy commented Sep 26, 2023

Regarding your implementation, your selector doesn't select anything yet? Hence it will update on all state changes? Perhaps you can get away with it if you useMemo a part of the state you're going to use in your UI.

No actually, the slector is passed from the user of this hook, for example you can use this hook like:

useUppySelector(uppyInstance, (state) => state.files)

In this case, the state in this hook is only about files, only when files updated the component using this hook will rerender. Although the listener in useEffect hook will run every time uppy state updated, but it only update component state when selector result changed.

Note this is just a prototype implementation, I suppose I will need to use useSyncExternalStore to make it work for defered state.

@Murderlon
Copy link
Member

Thanks for sharing. I'll experiment with your prototype soon to see if we can publish something like this with @uppy/react.

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 a pull request may close this issue.

2 participants