-
-
Notifications
You must be signed in to change notification settings - Fork 5
feat: types for prerender options and result; better result checks #29
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
base: master
Are you sure you want to change the base?
Conversation
|
@rschristian check it out, please 🙏 |
bb095cf to
75046d8
Compare
rschristian
left a comment
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.
From a quick skim, I'm not sure any of this is useful? It's a lot of minor formatting changes or just moving around jsdoc casts, neither of which are valuable.
|
@rschristian it's because of prettier. looks like nobody called "pnpm format" for a long time. I can format all the code in additional commit, it you want The sense of PR is in exported types |
|
Don't change the formatting at all please, revert the unrelated changes entirely and then I'll take another look. |
75046d8 to
40d3251
Compare
|
Returned formatting |
| result.renderTarget == 'body' | ||
| renderTarget == 'body' | ||
| ? '`renderTarget` was not specified in plugin options and <body> does not exist in input HTML template' | ||
| : `Unable to detect prerender renderTarget "${result.selector}" in input HTML template`, | ||
| : `Unable to detect prerender renderTarget "${renderTarget}" in input HTML template`, |
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.
Oof, good spot on both (or good spot from the types). Changed this API ages ago and I guess it slipped through.
| export interface PrerenderResult { | ||
| html?: string; | ||
| head?: Partial<Head>; | ||
| links?: Set<string>; | ||
| /** | ||
| * @description Caution: should be a valid JSON object | ||
| */ | ||
| data?: any; | ||
| } |
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 you spotted above in the source file, this should be a union with a string. return '<h1>Hello World</h1>'; is perfectly valid too.
| /** @type {import('./types.d.ts').PrerenderOptions} */ | ||
| const options = { ssr: true, url: route.url, route }; | ||
| result = await prerender(options); |
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.
There's no need for this line to change
| /** @type {import('./types.d.ts').PrerenderOptions} */ | |
| const options = { ssr: true, url: route.url, route }; | |
| result = await prerender(options); | |
| result = await prerender({ ssr: true, url: route.url, route }); |
|
|
||
| // Add any discovered links to the list of routes to pre-render: | ||
| if (result.links) { | ||
| if (typeof result === 'object' && result.links) { |
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.
There's no need for this line to change
| if (typeof result === 'object' && result.links) { | |
| if (result.links) { |
async function prerender({}: PrerenderOptions): Promise<PrerenderResult | string>