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(gatsby): New overlay for DEV_SSR #31061

Merged
merged 29 commits into from
May 7, 2021
Merged

feat(gatsby): New overlay for DEV_SSR #31061

merged 29 commits into from
May 7, 2021

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Apr 26, 2021

Description

This is for DEV_SSR flag (Discussion: #28138)

Previously the error overlay (when the page didn't successfully ssr'ed) consisted out of a HTML page served by express with helpful information. But that wasn't tied into our already existing Fast Refresh overlay we use throughout Gatsby.

Instead of serving all the information in the HTML a client-only shell is served that pushes to the window._gatsbyEvents queue.

Screenshots

image

[ch29833]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Apr 26, 2021
@LekoArts LekoArts added topic: ssr and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Apr 26, 2021
@LekoArts LekoArts marked this pull request as ready for review April 28, 2021 13:07
@@ -38,7 +38,7 @@ describe(`SSR`, () => {
const pageUrl = `http://localhost:8000/bad-page/`
// Poll until the new page is bundled (so starts returning a non-404 status).
const rawDevHtml = await fetchUntil(pageUrl, res => {
return res.status === 500
return res
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't explicitly send 500 status anymore as other errors (runtime, build, graphql) also don't do that

line ? `:${line}:${column}` : ``
} to resolve the error.`,
text: (context): string =>
stripIndent(`The path "${context.path}" errored during SSR.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's a WARNING the docsUrl is unused. Also added stripIndent

Copy link
Contributor

Choose a reason for hiding this comment

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

stripIndent won't do anything here because it looks for shortest indentation and removes that from every line - in here shortest indentation is empty string (because first line doesn't have any indentation)

Copy link
Contributor

Choose a reason for hiding this comment

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

above comment addressed in 9eb5a5b

export function prettifyStack(errorInformation) {
let txt
if (Array.isArray(errorInformation)) {
txt = errorInformation.join(`\n`)
} else {
txt = errorInformation
}
const generated = Anser.ansiToJson(txt, {
return Anser.ansiToJson(txt, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was seeing weird behavior with removing the first line for all different errors. The underlying error format must have improved so this isn't necessary anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

🤞 This doesn't give me much confidence 😛 Maybe it happened because of #30801

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was seeing weird behavior with removing the first line for all different errors.

Should rephrase that: The indentation for the now first line was slightly incorrect as the previously existing new line wasn't there anymore.

The PR you linked might have caused this, yeah

packages/gatsby/src/commands/build-html.ts Show resolved Hide resolved

const startWorker = (): any => {
const startWorker = (): JestWorker => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing TS errors in this file

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Do you want to add e2e tests so the buttons like refresh & skipSSR do actually work?

@@ -5,34 +5,38 @@ Cypress.on(`window:before:load`, win => {

const runTests = () => {
it(`should redirect page to index page when there is no such page`, () => {
cy.visit(`/redirect-without-page`).waitForRouteChange()
cy.visit(`/redirect-without-page`, {
failOnStatusCode: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left these in as in the future when we'd send 404 for the not found page those would fail

This was referenced Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: DX Developer Experience (e.g. Fast Refresh, i18n, SSR, page creation, starters)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants