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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: Upgrade Gatsby #138

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chore: Upgrade Gatsby #138

wants to merge 3 commits into from

Conversation

tlgimenes
Copy link
Contributor

What's the purpose of this pull request?

This PR upgrades gatsby to the latest version

@vercel
Copy link

vercel bot commented Jul 6, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
gatsby-store-storybook ✅ Ready (Inspect) Visit Preview Jul 6, 2022 at 2:08PM (UTC)

@tlgimenes tlgimenes added the Features New feature or request label Jul 6, 2022
@tlgimenes tlgimenes requested a review from a team July 6, 2022 12:45
@vtex-sites
Copy link

vtex-sites bot commented Jul 6, 2022 •
edited
Loading

Preview is ready

This pull request generated a Preview

👀   Preview: https://sfj-ac93b59--gatsby.preview.vtex.app
🔬   Go deeper by inspecting the Build Logs
📝   based on commit ac93b59

@vtex-sites
Copy link

vtex-sites bot commented Jul 6, 2022 •
edited
Loading

Lighthouse Reports

Here are the Lighthouse reports of this Pull Request

📝 Based on commit ac93b59

Lighthouse Report by page
📎   /
📎   /apple-magic-mouse/p
📎   /office

eduardoformiga
eduardoformiga approved these changes Jul 12, 2022
View reviewed changes
Copy link
Member

@eduardoformiga eduardoformiga left a comment

Choose a reason for hiding this comment

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

LGTM!

gatsby-config.ts
@@ -18,6 +18,7 @@ const gatsbyConfig: GatsbyConfig = {
},
flags: {
FAST_DEV: true,
DEV_SSR: false,
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity: why is it false?

Now you can add the DEV_SSR flag to your gatsby-config file so you can spot and fix SSR errors (like trying to access the window object) during development.

https://www.gatsbyjs.com/docs/reference/release-notes/v4.18/#notable-bugfixes--improvements:~:text=spot%20and%20fix%20SSR%20errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I enable devSSR everything breaks with a hydration mismatch error. Somehow Gatsby does not render the page.tx, only the layout.tsx, creating this mismatch

Copy link
Contributor

Choose a reason for hiding this comment

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

That's too bad because this seems like a useful feature. :(

Just the other day, a store was using an unprotected window call inside the PDP that was causing an SSR error, but it did not error during development. It errored (window is not defined) only if they built & served. It feels like with this flag, it would have errored earlier during development.

filipewl
filipewl approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@filipewl filipewl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me as well! Left a quick note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eduardoformiga eduardoformiga eduardoformiga approved these changes

@filipewl filipewl filipewl approved these changes

Assignees
No one assigned
Labels
Features New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tlgimenes @filipewl @eduardoformiga