RFC: Dropping data skeletons in React
vicary opened this issue · 14 comments
There have been discussions on the necessity of adding MakeNullable<T> in codegen.
It was deemed necessary back then because the original design was to always return undefined placeholder data to favor skeleton/glimmar loaders.
Since React 18 is officially out, we can count on suspense mode now. I think it is time to ask this question again.
If this is a favorable change, suspense mode will be the ONLY mode, hence dropping undefined as data skeleton altogether.
This would be a breaking change, I would like to know the impact to existing users.
Do you think it is a good idea?
Examples
Given the following GraphQL schema
type Query {
foo: String!
bar: String
}It will produce the following types:
const query = useQuery();
query.foo; // string
query.bar; // string | nullYes please! Would greatly improve the type safety of the library.
Wouldn’t this mean no more optimistic UI? In that before we could animate in a next page and show skeletons, whereas with this change you’d have a click… then wait… then show the next page?
Could we do just two different hooks for this? useQuerySuspense or something?
@natew The idea is that <Suspense> has a fallback prop for skeletons. But it does take efforts to migrate if you have a top-level skeleton component relying on the data placeholders.
I think I can retain both with some generic type magics, learning from tRPC <10 we would want to reduce recursive type inferences for IDE perf.
I get that but those fallbacks are very ad hoc, you have to basically render a totally fake page then rather than having your components automatically render a skeleton when the data is undefined. It’s a pretty neat feature of gqty imo that you get skeleton pages that 100% match your final content for free. But maybe I’m missing something.
@natew I would want upcoming changes to go towards a more modular design, having the core stays slim. Unfortunately the new React.use() API only works with suspense, so suspense has to be a first-class citizen.
Optimistic UI is still possible in an opt-in manner, where it becomes a wrapper over the suspense API and returns a data placeholder from another internal helper. Such that migration effort should be minimal and progressive.
What do you think?
+1 for natew.
If I did get it right, it'd be an extra effort to write a fallback that will access all the fields that the main subtree is accessing. It is also a potential source of issues when those will be out-of-sync.
@vasyas The selection tree will be tapped directly from the observers, this is the same hooks to build our queries when we fetch. I aim to reduce friction as much as possible while moving forward, so you may very well imagine a new option in your gqty.config.js which may or may not be called legacy: true which triggers a slightly different generated client.
Other then a new option which makes the data skeleton opt-in, nothing else will be changed.
EDIT: In fact the suspense: true option may already covers and has implied what we need here.
@noverby There is castNotSkeleton and castNotSkeletonDeep.
But use with care, only when you are using both prepare and suspense in useQuery() will it actually skip the first skeleton render in React.
@vicary Let me know if I'm understanding wrong but castNotSkeleton and castNotSkeletonDeep will basically just remove all undefined, not really checking if the field is nullable or non-nullable right? That's why you have to be careful with it.
Isn't there a way to only let MakeNullable only apply when the field is nullable (i.e. doesn't have the ! type), if you use suspense it shouldn't be dangerous right?
@Negan1911 There are only specific combinations of options that we can safely drop the data skeleton, i.e.
useQuery()withprepareandsuspenseenableduseTransactionQuery()withsuspenseenableduseLazyQuery()anduseMutation()after invoked
This RFC tries to enforce such a usage when you enable suspense globally. It also act as a step to embrace the upcoming React Server Components, where skeletons most likely lives inside <Suspense /> blocks, so you would want a way to always render skeleton data instead of a proxy.
For existing users without suspense, I am planning to keep the behaviour unchanged.
@vicary Let me know if/how I can help you get this RFC started, I just came from other clients that were a pain (not only querying, but dealing with cache) and GQTY looks really promising 🙌🏻
@Negan1911 Really appreciate it. Be prepared because in GQty we are dealing with a lot of meta-programming, proxies, recursions and cache normalization.
DM me in Discord if you are that committed to start digging the rabbit hole, I'll answer your questions along the way.
To implement this RFC, we'll need to tweak the CLI codegen, add a new helper in core, and throw in React when appropriate.
codegen
Add MakeNullable only when suspense is disabled.
The helper function
You'll need to understand the concept of the new core to know what this sentence means --- "A Skeleton helper is only an accessor with an empty cache as it's scoped context."
The new core
I just went through all of these and trimmed down and much as possible in #1544, hopefully it'll allow more good-first issues in the future. It is still pre-alpha with a lot of testing to be done, but the foundation is there.
What now?
The current plan is to export two schema types,
- One with optional scalars to reflect the state before fetch, and
- Another schema without optional to reflect the results.
Where?
A new non-nullable schema
This can only be applied to those places we are confident that data exists, such as
- The return type of
resolve - The yield type of
subscribe - The return type of
useQueryONLY whenprepareandsuspenseare specified.
The current schema
This is the current schema and it will stay unchanged for the rest of the API.