-
-
Notifications
You must be signed in to change notification settings - Fork 506
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
refactor: Use prerender data API #1192
Conversation
@@ -12,12 +11,11 @@ if (typeof window !== 'undefined') window.githubStars = githubStars; | |||
|
|||
export default class GithubStars extends Component { | |||
state = { | |||
stars: localStorageGet('_stars') || fallbackData.preactStargazers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting/setting from/to localstorage here seems unnecessary? I don't see any obvious benefit to doing so.
@@ -200,7 +200,7 @@ export default class Stars extends Component { | |||
} | |||
} | |||
// --repl-after | |||
render(<Stars />, document.getElementById("app")); | |||
render(<Stars repo="preactjs/preact" />, document.getElementById("app")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, drive-by issue that I ran into whilst testing:
While our usage example on the home page included the prop, the repl-after here was missing it. This was problematic as it'd attempt to fetch https://github.com/undefined
if the user tried to open the example in the repl. Must've gotten missed when we added the repl-before & after stuff.
import { TutorialContext, SolutionContext } from './contexts'; | ||
import { ErrorOverlay } from '../repl/error-overlay'; | ||
import { parseStackTrace } from '../repl/errors'; | ||
import cx from '../../../lib/cx'; | ||
import { InjectPrerenderData } from '../../../lib/prerender-data'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a relic of the Preact-CLI setup, InjectPrerenderData
was linked to a getPrerenderData
function that hasn't been in use since we switched over. Injecting therefore did nothing.
// fetch latest stargazer count | ||
const { default: repoLambda } = await import('./lambda/repo.js'); | ||
const { stargazers_count: stargazersCount } = await (await repoLambda()).json(); | ||
prerenderData.preactStargazers = stargazersCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is very far from crucial (only used in a demo at the very bottom of the home page), I had originally left it out & we just used a hard-coded fallback. Now that this PR introduces a better prerender data setup, however, I felt like it was worth fetching on build & ditching the old hard-coded count.
state = { | ||
stars: localStorageGet('_stars') || fallbackData.preactStargazers | ||
}; | ||
export default function GitHubStars({ user, repo, simple, children }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted to a function as it simplified consuming the context
Now that
@preact/preset-vite
supports prerender data, we can remove the hard-coded fallbacks and use the injected prerender data which is likely to be kept closer in-sync. This PR also ditches passing prerender data around withglobalThis
(sorry) for a prerender context provider.