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

chore(dx): Improve errors and warnings during Cell prerendering #6206

Merged
merged 15 commits into from
Aug 15, 2022

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented Aug 12, 2022

What does this PR do?

  • Better error when there's a routeHook error
    Routehook error

  • Better error when using a a protected query in a cell that is being prerendered
    Protected query-2

  • Only warn when graphql function has been moved, and fallback to Loading state for Cells

Graphql handler moved

  • Better logging of whats been prerendered
  • Make sure output of prerender during rw build is preserved
    Prerender output during build
  • Updates prerender docs with examples and better organisation of concepts

Closes #6201

@nx-cloud
Copy link

nx-cloud bot commented Aug 12, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a1088f2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 14 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Aug 12, 2022

Deploy Preview for redwoodjs-docs ready!

Name Link
🔨 Latest commit a1088f2
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62faa508adf40f00096f67f8
😎 Deploy Preview https://deploy-preview-6206--redwoodjs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@dac09 dac09 added the release:chore This PR is a chore (means nothing for users) label Aug 12, 2022
@dac09 dac09 requested a review from Tobbe August 12, 2022 12:46
docs/docs/prerender.md Outdated Show resolved Hide resolved
docs/docs/prerender.md Outdated Show resolved Hide resolved
docs/docs/prerender.md Outdated Show resolved Hide resolved
docs/docs/prerender.md Outdated Show resolved Hide resolved
packages/cli/src/commands/prerenderHandler.js Outdated Show resolved Hide resolved
packages/prerender/src/runPrerender.tsx Outdated Show resolved Hide resolved
packages/web/src/components/createCell.tsx Outdated Show resolved Hide resolved
dac09 and others added 3 commits August 15, 2022 13:08
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
…9/redwood into fix/cell-prerender-graphql-fallback

* 'fix/cell-prerender-graphql-fallback' of github.com:dac09/redwood:
  Stylistic suggestions
  Leverage PROJECT_CWD to set cwd in yarn 3 bin "proxies" (redwoodjs#6199)
  chore(cli): Add additional tests for sdl generator (redwoodjs#6200)
Copy link
Collaborator Author

dac09 commented Aug 15, 2022

Fair comment. I'll rename!

@dac09
Copy link
Collaborator Author

dac09 commented Aug 15, 2022

@Tobbe any thoughts on how we could add additional tests jest tests to prevent regression on this behaviour?

I'm guessing we have to wrap a cell in the provider and render it after giving it a value?

@Tobbe
Copy link
Member

Tobbe commented Aug 15, 2022

@dac09 I added to our smoke tests when I added cell prerendering. Maybe add something there that's testing protected queries?

Can we add jest tests for the runPrerender method? If we could, we could mock path.join() to return a broken path to the graphql file, and test that it prerenders the loading state

Copy link
Collaborator Author

dac09 commented Aug 15, 2022

Nah - the runPrerender is too involved, and need webpack, etc. to do it.

I was thinking more if we could add unit tests that checks the “prerendered” behaviour of createCell (so just in the react world. without all other stuff)

Copy link
Member

Tobbe commented Aug 15, 2022

Ohh, yeah, that'd be pretty easy. Just prep the context with the required data.
You should be able to explicitly call createCell in a component, render that with that global PRERENDERING flag set and then expect on screen

@dac09
Copy link
Collaborator Author

dac09 commented Aug 15, 2022

I won't do it in this PR, because I think we need tests for this anyway, but I'll keep it on my list for later

@dac09 dac09 mentioned this pull request Aug 15, 2022
18 tasks
@Tobbe Tobbe enabled auto-merge (squash) August 15, 2022 20:18
@Tobbe Tobbe merged commit b9110e9 into redwoodjs:main Aug 15, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Aug 15, 2022
@jtoar jtoar modified the milestones: next-release, v3.0.0 Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Handle when graphql handler not found in cell prerendering
3 participants