[Proposal] Lazy load views with suspense

[emmatown]

Related to #744

Note that this PR should be merged after #748 #751

[JedWatson]

Code reviewed the WIP, this looks really nice 👌

[emmatown]

This PR is basically ready but there's some test failures that I've been trying to reproduce and find the issue but I've had no luck so if anyone else has time to try to find the issue, that would be awesome.

[jesstelford]

I'd like to put a hold on this PR before it gets merged.

The code here all looks great, but the introduction of Suspense negates the ability to do server side rendering.

There is an alternate implementation in #745 which doesn't have that problem.

I am also looking at cherry-picking the best of this PR & #745 into the cli-build branch which is moving the Admin UI to be powered by Next.js for all the reasons outlined here (if you have access to Slack):

It will give us a number of benefits:

1️⃣ Remove dependence on react-router throughout @arch-ui packages
2️⃣ Add Server Rendering
3️⃣ All the benefits of many many contributors to the Webpack config for dev & prod
4️⃣ Serverless bundling
5️⃣ Static builds

[emmatown]

As discussed in slack, I still strongly disagree with doing server rendering and am doubtful of the performance benefits that we’ll get here.

Also, I think it would be quite easy to do a static build of the admin ui without using next.js. (I’m happy to implement it)

Also, static builds and serverless bundling are effectively the same thing since if we can have a static build we can deploy it to a serverless target.

[JedWatson]

I've also got real concerns about using Next.js for the Admin UI; largely because (as things stand) the scalability of next is based on different principles. We'll end up with three "pages", two of which will be really heavy because they're the list and item pages which support all the lists and fields configured in the project.

What we really want is for each List interface to be generated independently based on the fields it uses and (in the future) plugins it includes. We'll end up inventing our own mechanisms on top of Next because that will become necessary down the track.

A technique I've seen used elsewhere is to generate pages for Next.js on server start based on dynamic information (in our case this would be lists) so that optimised bundles are created and you get the benefits of the page-based splitting and prefetching.

But I don't think that's a road we should go down lightly. This will have nontrivial knock-on impacts to the future complexity of both developing the Admin UI ourselves, and Keystone users developing extensions to the Admin UI.

[jesstelford]

am doubtful of the performance benefits that we’ll get here.

I encourage you to understand next.js at a deeper level. It is the first tool which strikes an excellent balance between the pros and cons of both server & client rendering.

For example; it's possible to get the benefits of a page server rendered, while only downloading a subset of the JS thanks to dynamic imports and intelligent bundle splitting.

After working on multiple large scale projects, I cannot understate the benefits server rending brings; not just initial page load performance, but perceived performance, engagement metrics, SEO benefits (a non-issue for the Admin UI), accessibility benefits, and even down to things like reducing "random whitepages".

For a primer, I highly recommend this flow chart: https://kryogenix.org/code/browser/everyonehasjs.html

static builds and serverless bundling are effectively the same thing since if we can have a static build we can deploy it to a serverless target.

Deploying a client-side build still serves a client-side application. Serverless or not.

the scalability of next is based on different principles.

I'd love to hear more on this - I'm not aware of any scalability issues with next, but am very aware of all the scaling benefits that it provides. Particularly with the most recent "serverless" target that comes with 8.

We'll end up with three "pages", two of which will be really heavy because they're the list and item pages which support all the lists and fields configured in the project.

This would be the case whether it's a client side rendered app or a server side rendered app. It's a fact of the product, not of the tool used to build.

And we've already seen two separate examples where the pages aren't actually "heavy"; #745 & #749 thanks to lazy loading. The cli-build branch has a slightly modified strategy from #745 so it supports server rendering meaning we get the best of both worlds: a very fast initial page load, plus a quick TTI, and lazily load the complex parts of the UI as the user navigates.

We'll end up inventing our own mechanisms on top of Next because that will become necessary down the track.

Why would that be necessary? Next supports (and encourages) using dynamic imports. As it stands, I know of no concrete example of an Amin UI feature which couldn't be served by next.js

A technique I've seen used elsewhere is to generate pages for Next.js on server start based on dynamic information (in our case this would be lists) so that optimised bundles are created and you get the benefits of the page-based splitting and prefetching.

But I don't think that's a road we should go down lightly.

We already do this today. The field-views-loader does exactly what you're describing; aggregating all the list and field data into a format that then gets consumed by the frontend, and also impacts the way it's bundled.

This will have nontrivial knock-on impacts to the future complexity of both developing the Admin UI ourselves, and Keystone users developing extensions to the Admin UI.

Which is my reason to use Next; let Next.js (and the huge community there) handle that complexity so we can build our app on top of that solid foundation.

[emmatown]

Before I address more things, I'd like to make sure that some the assumptions I'm making about how we would use next and what users of the Admin UI care about are correct so please tell me I'm wrong or you disagree about anything here.

• if we use Next, we would do static builds so we can deploy to things like netlify.
  • therefore we would also do data fetching on the client?
• the Admin UI will generally be used on desktop browsers(generally, not always) and for each instance of keystone there will generally be a relatively small number of people who use it oftenish
• Most(practically all?) production keystone instances will have authentication on the Admin UI
• Performance in production is much much more important than development so performance tests in development shouldn't be used to decide which thing is more performant
  • sub thing with this which i feel strongly about: benchmarks and metrics are useful tools that can aid in performance work but are imperfect ways of measuring performance and UX that have to be aided with other tools and performance has to be considered in the wider context of how something is used(not directed at any prior discussion here, i know you haven't been doing this but just want to make sure nobody(including me) naively uses benchmarks and metrics as justification here (i've naively used benchmarks and metrics in the past to optimise things and it's always turned out badly))
• The majority of the users of the Admin UI are using it so they can read or change data and the amount of time that it takes to find or change their data is the most important performance metric to them and it's okay if other performance metrics are worse if it makes the amount of time it takes to get to and edit their data better.

[jesstelford]

if we use Next, we would do static builds so we can deploy to things like netlify. therefore we would also do data fetching on the client?

Yes, netlify is one target. Next supports 3 output types:

• [CSR] Static html builds (similar to Gatsby sans the GraphQL data sourcing stuff)
• [SSR] Serverless bundles ready for deploying on pretty much any JS runtime (Cloudflare Workers, AWS Lambdas, an instance of node, etc)
• [SSR] A "just node" build which still requires a full node environment

In the CSR case, all data fetching would be done client side.

In the SSR cases, data fetching can be done on both server and client (thanks to Next's getInitialProps()).

the Admin UI will generally be used on desktop browsers(generally, not always)

This is not an assumption I want to make. Ideally I would make the inverse assumption: It will be used a lot on Mobile, which then forces us to think through performance and UX challenges. Scaling up from mobile is much easier than scaling down from desktop.

for each instance of keystone there will generally be a relatively small number of people who use it oftenish

There are at least 2 Thinkmill clients I know of who employee people full time to do work within the Admin UI.

I also use the Admin UI frequently for Cete.

Most(practically all?) production keystone instances will have authentication on the Admin UI

Honestly no idea on this one. @JedWatson any numbers on KS4 instances which has auth enabled/disabled?

Performance in production is much much more important than development

Agreed. I wold never turn down a chance to get perf benefits in dev mode, but prod should be our focus 👍

The majority of the users of the Admin UI are using it so they can read or change data

This really is the only function of the Admin UI. It's a good thing to think this through some more, and we could possibly come up with some more concrete and specific flows which might help inform areas of focus.

👆 These are all in response to the assumptions you've surfaced. I want to point out that there is more that goes into building a successful product. A couple of them off the top of my head:

• Long term maintenance & upkeep. As with all code; it will inevitably get out of date / have more and more tweaks and modifications made over time making it more complex or harder to reason about. This is the main reason why Keystone was rebuilt between version 4 & 5. By sectioning off portions of the application with clear and distinct APIs, it helps compartmentalise them in a way that they can evolve independently. This is very similar to "Thinking in Components" in React. Doing this at the product level means things like picking a framework to handle the building, while creating the application on top. Then you can iterate the framework and the application independently (provided there are no major breaking changes in the framework). This then leads to the second point:
• The ability for new contributors to engage / enhance / fix things. If someone wants to contribute and notices it's powered by next, they can forget about having to understand how next works internally and focus just on the application. Or vice-versa; they can focus on how to contribute to next without worrying about the application, etc.

[jesstelford]

I'd also like to pose a question: If we can have SSR + CSR with similar (or equal) performance / UX / etc, is there any reason not to?

[emmatown]

if we use Next, we would do static builds so we can deploy to things like netlify. therefore we would also do data fetching on the client?

Yes, netlify is one target. Next supports 3 output types:

• [CSR] Static html builds (similar to Gatsby sans the GraphQL data sourcing stuff)
• [SSR] Serverless bundles ready for deploying on pretty much any JS runtime (Cloudflare Workers, AWS Lambdas, an instance of node, etc)
• [SSR] A "just node" build which still requires a full node environment

In the CSR case, all data fetching would be done client side.

In the SSR cases, data fetching can be done on both server and client (thanks to Next's getInitialProps()).

Just to be absolutely clear, you're proposing that we use both the CSR and SSR output types of next?

we could possibly come up with some more concrete and specific flows which might help inform areas of focus.

Agreed, we should do this.

the Admin UI will generally be used on desktop browsers(generally, not always)

This is not an assumption I want to make. Ideally I would make the inverse assumption: It will be used a lot on Mobile, which then forces us to think through performance and UX challenges. Scaling up from mobile is much easier than scaling down from desktop.

This was based on the fact that the current state of admin UI on mobile is that it's basically unusable and that @JedWatson said in slack, the Admin UI should work on mobile were possible but not where difficult so I inferred that it wasn't our highest priority. While I agree that it would be great to make the assumption that it will be used a lot on mobile but I'm questioning if being forced to think through all the performance and UX challenges are worth it if it's not the highest priority. If I'm wrong about mobile not being a high priority then I completely agree with you but the impression I've gotten from @JedWatson is that it isn't.

• Long term maintenance & upkeep. As with all code; it will inevitably get out of date / have more and more tweaks and modifications made over time making it more complex or harder to reason about. This is the main reason why Keystone was rebuilt between version 4 & 5. By sectioning off portions of the application with clear and distinct APIs, it helps compartmentalise them in a way that they can evolve independently. This is very similar to "Thinking in Components" in React. Doing this at the product level means things like picking a framework to handle the building, while creating the application on top. Then you can iterate the framework and the application independently (provided there are no major breaking changes in the framework). This then leads to the second point:
• The ability for new contributors to engage / enhance / fix things. If someone wants to contribute and notices it's powered by next, they can forget about having to understand how next works internally and focus just on the application. Or vice-versa; they can focus on how to contribute to next without worrying about the application, etc.

I strongly strongly agree that long term maintenance & upkeep and the ability for new contributors to engage / enhance / fix things are really important things and that abstracting away parts of app in clear ways is awesome. I don't agree that Next would help in keystone's case because I think that the problems which the Admin UI has are different to the problems that Next is solving. (gonna expand on this in another comment) WRT contributing to the app and not having to care about config and dev server things and such, I think adding next would have the opposite effect since rather than people only having to think about rendering some React components on the client as it is right now, people would have to worry about the complexity of SSR, Next's routing rather than one of the de-facto standard routing libraries for React and all of the abstractions that Next adds like getInitialProps and the pages folder and etc. I also think that we would have to add more abstractions on top of Next to do everything well as @JedWatson said. (also gonna expand on how I think we would have to do it if we wanted to do it well in another comment)

I'd also like to pose a question: If we can have SSR + CSR with similar (or equal) performance / UX / etc, is there any reason not to?

Assuming that it didn't significantly diminish on maintainability and isn't significantly more complex along with equal or better UX/performance/etc then absolutely(I disagree with most of these assumptions, gonna expand more in another comment) then I would completely agree with doing SSR without a doubt.

[emmatown]

UX with SSR

There are a couple things in the Admin UI that rely on local storage for displaying things on the initial render. (The filters when not provided in the url, the nav component, maybe other things i'm not thinking of right now?) If we did SSR, these things would create a worse user experience as the incorrect layout and state would initially be displayed and then be updated to the correct state after the JS loads and the app has mounted.

In slack, when I mentioned this problem, you proposed rendering at a higher level than the nav component but this would mean that we would essentially not be server rendering anything and based on you saying that we would do SSR with data fetching in some cases, I'm assuming you're proposing we render deeper than the nav, please lmk if this assumption is wrong.

Lazy loading field type views

I don't believe that the approach taken in #745 means it's easy to server render. While I've said that Suspense doesn't work in SSR right now, that doesn't mean that #745 works in SSR. A big problem with lazy loading in SSR right now is that you need to have the same assets available when you hydrate the page, this is a solvable problem but it requires a fair bit of complexity. Next(/react-loadable which is what Next uses) has solved this for lazy loading React components but not all of the field type views are React components so we couldn't use next/dynamic, we'd have to build our own stuff on top of next to find the bundles that are needed for the initial render and then use webpack internals to synchronously require the modules on client. This is of course assuming that we won't encounter even more problems doing this that I haven't thought of. Doing this would also complicate statically rendering the Admin UI because we'd have to dynamically generate the list and item pages and write them to disk for each list so that they have the appropriate bundles for the initial render. (as @JedWatson said)

Side note that doesn't affect this but I think is cool and exciting: This is something that the React team is working on solving really well with suspense where a React tree will be able to partially hydrate based on where the suspense components are in the tree so children of suspense components will only hydrate after their children have finished suspending.

---

What I'm trying to say with all of this is similar what I said when @jossmac originally suggested doing SSR, there are lots of problems that we would have to solve if we did SSR and we probably could solve the majority of them but not without introducing a massive amount of complexity on top of Next which would IMO negate any DX benefits we would get from the good webpack defaults and have parts of the Admin UI that have worse UX.

[emmatown]

I found out why some of the access control tests were failing, the tests were looking for the plural version of the list name in the title on the list page and before this PR, it found the plural version because it checked the title before the data loaded but I think this PR subtly changed timing things which can't be be seen by people but made the data finish loading before cypress could find the title since once the data loads, the title changes to 1 singular list name. So I fixed it by changing the tests from searching for the plural to the singular(and if it found the plural, it would still work).