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

RFC/Proof of concept: Adopt Suspense and Concurrent Mode roughly following Relay's Entry Points #2922

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

taneliang
Copy link
Member

@taneliang taneliang commented Oct 21, 2020

Context

TL;DR we remove react-loadable and React Router and replace them with a DIY solution with Suspense and Relay-like Entry Points (or at least my understanding of entry points). Concurrent Mode is needed for useTransition, but I think we can remove it if we don't like it/it causes problems/it's too unstable.

This PR is very much a WIP. Only the http://localhost:8080/timetable/sem-1, http://localhost:8080/today, and all module pages work at this moment. Many things are also broken, including hot reload. What works are loading one URL (e.g. the timetable) and hovering over another to preload its code. The today page will also start loading its data on click, before the Today component renders.

Key goals

  • Modernize codebase to use latest React features.
  • Centralize and formalize bundle loading and data preloading, to make both bundle splitting and implementing render-as-you-fetch easier:
    • We currently split our bundle by routes, and our navbar/sidebar preloads those routes on hover. Unfortunately, that's the only place that preloads on hover, partly because it was inconvenient to trigger preloads throughout the app. This PR moves preloading into new PreloadingLink and PreloadingNavLink components (which wrap React Router's Link and NavLink respectively), allowing all links in the app to automatically preload on hover.
    • Render-as-you-fetch will improve load times, as currently (in many places) we start fetching data on component mount. We did have data preloading for venue locations, but that was an isolated case. This PR centralizes it into entry points; specifically, the new React Router router will execute the prepare function for all entry points that are displayed in the current route.
  • Lay groundwork for future GraphQL integration on the frontend. The entry point approach is perfect for root components that can gather the data requirements of its entire tree and preload everything.
  • Remain agnostic to data sources. Although we use Relay's approach, the prepare functions are simply functions that preload data, which are fed into components as a prepared prop.

Concerns

  • Route definitions are currently really long. I'll probably try to solve this by moving prepare definitions closer to their modules.
  • Because NUSMods is already pretty performant, the changes in this PR is likely not going to have any obvious effect on UX. Good for (my) learning though 😆

Implementation

The general idea is based on https://github.com/relayjs/relay-examples/blob/master/issue-tracker. I suspect that example repo follows old-ish FB practices, but I don't think any current approach they're using will be too different from it.

  • Converted some class components to functional ones, with some untested changes. There may be many bugs caused by this PR, oops.

GIFs

Fresh load without throttling Fresh load on simulated fast 3G
suspense-no-throttling suspense-fast-3g

Preloading on hover:

This is similar-ish to existing behavior, but now done by the Link component, and the venue data preload now happens on click instead of on hover.

suspense-network-activity

Known Issues

  • Error handling is considerably worse than before, as they're now caught by generic error boundaries instead of being rendered at components that know what errors to expect. We'll need to write smarter error pages that can distinguish between different error types and/or add error boundaries closer to where errors are expected.
  • Entry point getPreparedProps functions may be called multiple times when navigating between pages. Ideally we'll fix this by calling getPreparedProps once and feeding its result into the component, but React Router's preload doesn't do that, and it seems impossible to match preloaded data to its component after. I think this is a reasonable limitation: entry points also have a disposePreparedProps function, which can invalidate resources when the entry point unmounts.
  • A bit dumb to have 2 different resource implementations with different APIs and details.
  • ScrollToTop is overeager when navigating back/forwards on module pages. I suspect this is because ModulePageContent is now remounted.
  • History replacement is broken in a few places. This is likely because of some breaking changes in React Router v6.

Out of scope

  • Moving new functional components that use connect to react-redux hooks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant