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

Upgrade to Gatsby 3 and fix Heroku caching #2851

Merged
merged 148 commits into from
Oct 19, 2021
Merged

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Sep 24, 2021

Builds off of #2848, adding a script that stabilized build paths to ensure Gatsby doesn't clean its cache when not necessary, among some other changes to help speed up builds as much as we can.

Build times

Old packages (Gatsby 2)

Completely cold cache: 1937s
Automatic cache clear: 612s
Fully warm cache (script backport in #2898): 170s

New packages (Gatsby 4 beta)

Completely cold cache: 840s
Automatic cache clear: 283s
Fully warm cache: 91s

@julieg18
Copy link
Contributor

Weird, this is what site looks like on my end:

image

Do we need to try rebuilding it?

@rogermparent rogermparent temporarily deployed to dvc-org-test-caching-fi-nwea1h October 15, 2021 17:07 Inactive
@rogermparent
Copy link
Contributor Author

rogermparent commented Oct 15, 2021

That was weird, I got it too and a rebuild did indeed fix the fonts for me. The public folder on S3 was malformed, breaking the first build and triggering a full rebuild- not a good portent 😬 I think I remember some similar problems happening on the existing system though.

@rogermparent rogermparent marked this pull request as ready for review October 15, 2021 18:43
@julieg18
Copy link
Contributor

Everything looks great! Besides a small difference I noticed between dvc.org and the branch, I don't see any issues. This branch's dvc.org blog main image seems to be using dominantColor instead of blurred as a placeholder. Do we want this to be blurred instead?

@rogermparent
Copy link
Contributor Author

Nice catch! I don't have any preference between the two methods, though I'm a little curious as to why the default was changed from blur to dominant color- maybe lightening the amount of processing needed, or maybe it being deemed as looking better?

Either way, we can try setting the default placeholder to blurred to keep parity for now.

@rogermparent rogermparent temporarily deployed to dvc-org-test-caching-fi-nwea1h October 18, 2021 15:18 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-test-caching-fi-nwea1h October 18, 2021 15:55 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-test-caching-fi-nwea1h October 18, 2021 17:17 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-test-caching-fi-nwea1h October 18, 2021 18:12 Inactive
@rogermparent
Copy link
Contributor Author

rogermparent commented Oct 18, 2021

Default placeholder is now blur, and I did patch upgrades on some packages. I think this is as good as we're going to get for this PR!
Thanks for all the reviews @julieg18! 🙏

With an approval, we can merge and move to the next step of making sure the transition is as seamless as we can make it.

@rogermparent rogermparent temporarily deployed to dvc-org-test-caching-fi-nwea1h October 18, 2021 18:51 Inactive
Comment on lines +126 to +133
{
resolve: 'gatsby-plugin-sharp',
options: {
defaults: {
placeholder: 'blurred'
}
}
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point between our current pinned versions and now, the default placeholder got changed from blur to a "dominant color" strategy. I've changed the default back to blur for parity.

It doesn't seem there's any hit to build time for doing this, my first attempt looked like it but was a false positive.

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great job on this @rogermparent! We should probably make sure to keep a close eye on dvc.org and sentry errors after we merge this to be safe 😅

@rogermparent rogermparent temporarily deployed to dvc-org-test-caching-fi-nwea1h October 19, 2021 14:18 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-test-caching-fi-nwea1h October 19, 2021 15:18 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-test-caching-fi-nwea1h October 19, 2021 15:22 Inactive
@rogermparent rogermparent merged commit 3b3b52d into master Oct 19, 2021
@rogermparent rogermparent deleted the test-caching-fix branch October 19, 2021 15:29
@casperdcl casperdcl linked an issue Oct 27, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

image caching broken