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

feat(router): Implement routing with Suspense #8392

Merged
merged 27 commits into from
Jun 7, 2023
Merged

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented May 23, 2023

Working with @KrisCoulson to pull in the Suspense router into main.

Tasks:

  • Finish remaining tests
    • The old router tests don't work - we just need equivalent tests in the new style (analyzeRoutes.test and redirect.test)
  • Check that prerendered pages don't flash
    • Don't think there's a flash, but does look like there's hydration errors 🤷
  • Accessibility stuff - this is currently commented out because it was causing hydration errors on SSR. Without SSR maybe it works fine!
  • Should we reimplement auth redirects using replace instead of push (e.g. logout then go back)?
  • ok to remove the regular "loader" component?
    • Remove from Spec type def in utils.ts
    • Remove from babel-routes-autoloader plugin
  • Fix error on production build when navigating to the About page. This is a legit failure, I can reproduce locally (but the error logs aren't helpful) with webpack

@replay-io
Copy link

replay-io bot commented May 23, 2023

19 replays were recorded for 09e9cd2.

image 0 Failed
image 19 Passed

View test run on Replay ↗︎

@dac09 dac09 added the release:feature This PR introduces a new feature label May 23, 2023
@dac09
Copy link
Collaborator Author

dac09 commented May 24, 2023

19 replays were recorded for 6211e5c.
image 1 Failed

* [Smoke test with rw serve](https://app.replay.io/recording/f1a576db-7962-4d19-b4f4-e1a906a09762)page.textContent: Target closed
  =========================== logs ===========================
  waiting for locator('text=This site was created to demonstrate my mastery of Redwood: Look on my works, ye')
  ============================================================

image 18 Passed

View test run on Replay ↗︎

@KrisCoulson this looks like a legit failure. Only happens on build (currently only checking with Webpack).

To reproduce: yarn rw build && yarn rw serve, then navigate to about page

@KrisCoulson
Copy link
Contributor

@dac09 looking into this today

@KrisCoulson
Copy link
Contributor

All tests from the old router are now passing. There are two things to look into

currently we are skipping 1 test that handles page loading delay.
and we are getting a sort of regression on some prerender console errors due to react 18.

We sort of solved this with a temporary fix with release of React 18 but now we are using two Suspense boundaries so we are seeing some regressions.

@KrisCoulson
Copy link
Contributor

There is an issue with prerendering cell content that causes a mismatch. We can fix this issue by storing the cell prerender data in the head of a prerendered page and then prepopulate the apollo cache before page loads.

Current limitation is the cache that is being stored on the server during prerender is slightly different then the cache that is used client side. We need to figure out a way to use the same cache object

@KrisCoulson
Copy link
Contributor

I figured out how to actually get pre-render working. We can restore the Apollo cache to preload the data on the initial render this will make sure the first pre-rendered page you go to is server rendered and there are no React errors for mismatched data.

Note pre-render isn't actually 100% working on the main branch so this isn't technically Suspense router specific.

On thing this does include would be injecting a global variable in the head of pre-rendered pages.

We should discuss naming conventions of this variable. and if we want to move forward with this implementation here in the Suspense Router PR or break this out into its own issue.

@dac09 dac09 marked this pull request as ready for review June 7, 2023 07:14
@dac09 dac09 enabled auto-merge (squash) June 7, 2023 13:54
@dac09 dac09 merged commit 461a06b into main Jun 7, 2023
@dac09 dac09 deleted the feat/suspense-router branch June 7, 2023 14:31
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jun 7, 2023
jtoar pushed a commit that referenced this pull request Jun 8, 2023
Co-authored-by: Kris Coulson <kriscoulson@gmail.com>
@jtoar jtoar modified the milestones: next-release, v5.4.0 Jun 22, 2023
jtoar added a commit that referenced this pull request Jun 29, 2023
@jtoar jtoar modified the milestones: v5.4.0, v6.0.0 Jun 29, 2023
@jtoar jtoar added release:breaking This PR is a breaking change and removed release:feature This PR introduces a new feature labels Jun 29, 2023
Tobbe added a commit to Tobbe/redwood that referenced this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants