alleyinteractive / alley-scripts

A collection of scripts and utilities for Alley projects.
https://alley-scripts.alley.dev/
6 stars 1 forks source link

Block Editor Tools: `Suspense` support #457

Open renatonascalves opened 8 months ago

renatonascalves commented 8 months ago

Description

I just realized that none of the hooks, https://github.com/alleyinteractive/alley-scripts/tree/main/packages/block-editor-tools/src/hooks, have support for getting the status of a selector so that a developer can check if a resolver has finished its job.

A task that's particularly useful in scenarios where you need to know if data has been loaded or not.

This would involve a breaking change release, since the returned value of the hooks would be different.


For example:

const MyBlock = ({
 postID,
}) => {
  const post = usePost(postID, postType);

  // has the resolver finished getting the post?
  // What should the component do in the meantime?
  if (post) {
    ...
  }
};

More: https://redux.js.org/usage/deriving-data-selectors

Acceptance Criteria

kevinfodness commented 5 months ago

@renatonascalves is there a library that uses this pattern that you can reference? We've implemented this in a custom way in several of our repos, but if we are going to implement this in a more robust way, I'd like to follow some established patterns.

mboynes commented 5 months ago

Another option here would be to use useSuspenseSelect, which would give the option of leveraging Suspense without threat to being a breaking change.

renatonascalves commented 5 months ago

^ nice find! Had no idea. This would be ideal since it fallbacks to the default behavior.

renatonascalves commented 5 months ago

@kevinfodness Do you have any objection in using the useSuspenseSelect?

As part of the task, I'll add an example with and without suspense, to make it clearer for whoever is copying and pasting.

kevinfodness commented 5 months ago

The docs on this are terrible, so I went digging in the source code to figure out how this actually works. When the docs say that it "throws a Promise" it means that literally: https://github.com/WordPress/gutenberg/blob/3888f1e58528be5a641621ab171eba9f1e816928/packages/data/src/redux-store/index.js#L536

So I think this would be a breaking change, because users would need to wrap these hooks in a try/catch block, otherwise the thrown Promise would yield an uncaught Promise.

For example:

try {
  const post = usePost(postID, postType);
} catch ($promise) {
  // Do something with the unresolved state
}
renatonascalves commented 5 months ago

Yeah, it was too good to be true! Also since the Suspense tag in the parent component is also required to "catch" the promise.

I was reading the original pr that introduced it, https://github.com/WordPress/gutenberg/pull/37261, and the decision was exactly that. Not to fallback to the default behavior, otherwise it would not be truly supporting Suspense.

Having said all that, maybe it'd be better to have a Suspense version. Something like: useSuspensePost.

renatonascalves commented 4 months ago

The useSuspense... is also how some libraries are handing Suspense support.

See https://tanstack.com/query/latest/docs/framework/react/guides/migrating-to-v5#new-hooks-for-suspense

kevinfodness commented 3 months ago

Yeah, let's do that - making a separate version of each hook that would benefit from Suspense makes sense, lets users opt-in, and lets us customize the implementation of those hooks to use Suspense properly for each context (which may be different for different hooks).