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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: improve logging for _devPagesManifest.json loading failures #38046

Merged
merged 5 commits into from Aug 7, 2022
Merged

fix: improve logging for _devPagesManifest.json loading failures #38046

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d68044
fix: made _devPagesManifest.json loading resilient to failures
JoshuaKGoldberg Jun 27, 2022
d27ff1b
On second thought, make it even more verbose
JoshuaKGoldberg Jun 27, 2022
be68a96
Added error message description page
JoshuaKGoldberg Aug 7, 2022
0e9c2b9
Merge branch 'canary'
JoshuaKGoldberg Aug 7, 2022
c18a712
Merge branch 'canary' into resilient-promised-dev-pages-manifest
ijjk Aug 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 12 additions & 17 deletions packages/next/client/page-loader.ts
View file
Open in desktop
Expand Up @@ -75,23 +75,18 @@ export default class PageLoader {
if (window.__DEV_PAGES_MANIFEST) {
return window.__DEV_PAGES_MANIFEST.pages
} else {
if (!this.promisedDevPagesManifest) {
// TODO: Decide what should happen when fetching fails instead of asserting
// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Psst - may I suggest using @ts-expect-error instead? I would be very happy to send a PR migrating over! 馃檪

More broadly: I'd also suggest enabling typescript-eslint's prefer-ts-expect-error rule and, even more broadly, much more of typescript-eslint's rules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely open to migrating this over to ts-expect-error and investigating better TypeScript lint checks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super! For investigating more lint checks, I'd been chatting with @elsigh to try to get a more formal effort set up. Will check back in tomorrow!

this.promisedDevPagesManifest = fetch(
`${this.assetPrefix}/_next/static/development/_devPagesManifest.json`
)
.then((res) => res.json())
.then((manifest: { pages: string[] }) => {
window.__DEV_PAGES_MANIFEST = manifest
return manifest.pages
})
.catch((err) => {
console.log(`Failed to fetch devPagesManifest`, err)
})
}
// TODO Remove this assertion as this could be undefined
return this.promisedDevPagesManifest!
this.promisedDevPagesManifest ||= fetch(
`${this.assetPrefix}/_next/static/development/_devPagesManifest.json`
)
.then((res) => res.json())
.then((manifest: { pages: string[] }) => {
window.__DEV_PAGES_MANIFEST = manifest
return manifest.pages
})
.catch((err) => {
throw new Error(`Failed to fetch devPagesManifest: ${err}`)
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to throwing on failure would be to make the getPageList function return Promise<string | undefined>. I personally prefer informative return types rather than mysterious error throwing. However, I think this code path is only ever called in dev mode within code that already try/catches.

return this.promisedDevPagesManifest
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/next/shared/lib/router/router.ts
View file
Open in desktop
Expand Up @@ -1092,7 +1092,7 @@ export default class Router implements BaseRouter {
// The build manifest needs to be loaded before auto-static dynamic pages
// get their query parameters to allow ensuring they can be parsed properly
// when rewritten to
let pages: any, rewrites: any
let pages: string[], rewrites: any
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any

TypeScript would have caught this crash were it not for the any and/or !.

cc @delbaoliveira since we'd been chatting about this at Prisma Day - happy to help clean up these types! 馃檪

try {
;[pages, { __rewrites: rewrites }] = await Promise.all([
this.pageLoader.getPageList(),
Expand Down