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): Load static query results in Gatsby runtime #25723

Merged
merged 19 commits into from
Jul 20, 2020

Conversation

sidharthachatterjee
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee commented Jul 13, 2020

Static query results in Gatsby are currently imported as JSON by webpack. This means that static query results are included in your site's JavaScript bundles.

This is different from how page query results are written and loaded on the client by Gatsby. They are loaded by the Gatsby runtime instead and hence live outside of the webpack pipeline. This ensures that a single data change doesn't invalidate your entire site's JavaScript assets.

This PR makes the handling for Static Queries similar.

(This was previously opened as #23837 but then superseded by several more granular PRs of which this is the last)

@sidharthachatterjee sidharthachatterjee requested a review from a team as a code owner July 13, 2020 14:29
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 13, 2020
@sidharthachatterjee sidharthachatterjee added status: needs core review Currently awaiting review from Core team member type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement. and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jul 13, 2020
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

It looks good even if I have some comments. I'm not sure I'm entirely wrapping my head around it semantically (that's on me, not you).

packages/gatsby/cache-dir/ensure-resources.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/gatsby-browser-entry.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
Copy link
Contributor

@freiksenet freiksenet left a comment

Choose a reason for hiding this comment

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

Does this also need a develop side of this with static queries over websocket?

Also in-flight stuff is missing.

Do we have any runtime tests for this?

Otherwise LGTM, great work!

packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
@gatsby-cloud-staging
Copy link

gatsby-cloud-staging bot commented Jul 15, 2020

Your pull request can be previewed in Gatsby Cloud: https://build-3ce63284-7a05-4066-8bd5-bc8732a96c29.staging-previews.gtsb.io

Copy link
Contributor

@freiksenet freiksenet left a comment

Choose a reason for hiding this comment

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

Cleanup is missing from inFlight stuff and I'd personally unify and lightly abstract those, but it's also okay to also keep specific solution for both, as long as we clean it up.

packages/gatsby/cache-dir/ensure-resources.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/loader.js Outdated Show resolved Hide resolved
@sidharthachatterjee sidharthachatterjee removed the status: needs core review Currently awaiting review from Core team member label Jul 15, 2020
@ascorbic
Copy link
Contributor

The test failures seem to be genuine errors (or at least changes that need updating in the tests).

ascorbic
ascorbic previously approved these changes Jul 16, 2020
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Looks good!

freiksenet
freiksenet previously approved these changes Jul 16, 2020
Copy link
Contributor

@freiksenet freiksenet left a comment

Choose a reason for hiding this comment

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

Could you answer @pieh comment? Otherwise LGTM.

@sidharthachatterjee
Copy link
Contributor Author

Published a canary at gatsby@static-queries-out-of-webpack

@sidharthachatterjee sidharthachatterjee added the status: needs core review Currently awaiting review from Core team member label Jul 17, 2020
@sidharthachatterjee sidharthachatterjee removed the status: needs core review Currently awaiting review from Core team member label Jul 20, 2020
@sidharthachatterjee sidharthachatterjee merged commit b00c3df into master Jul 20, 2020
@sidharthachatterjee sidharthachatterjee deleted the feat/static-queries-out-of-webpack branch July 20, 2020 15:09
@DanailMinchev
Copy link
Contributor

Hello @sidharthachatterjee @freiksenet

I think this breaks integration with Storybook, for example, DanailMinchev/gatsby-starter-testing#31 :

The result of this StaticQuery could not be fetched.

This is likely a bug in Gatsby and if refreshing the page does not fix it, please open an issue in https://github.com/gatsbyjs/gatsby/issues
Error: The result of this StaticQuery could not be fetched.

This is likely a bug in Gatsby and if refreshing the page does not fix it, please open an issue in https://github.com/gatsbyjs/gatsby/issues
    at useStaticQuery (http://localhost:6006/vendors~main.b507b5bad99048f14b9e.bundle.js:30995:11)
    at Layout (http://localhost:6006/main.b507b5bad99048f14b9e.bundle.js:279:75)
    at oh (http://localhost:6006/vendors~main.b507b5bad99048f14b9e.bundle.js:43442:146)
    at Rj (http://localhost:6006/vendors~main.b507b5bad99048f14b9e.bundle.js:43550:496)
    at Qj (http://localhost:6006/vendors~main.b507b5bad99048f14b9e.bundle.js:43535:265)
    at Kj (http://localhost:6006/vendors~main.b507b5bad99048f14b9e.bundle.js:43535:194)
    at yj (http://localhost:6006/vendors~main.b507b5bad99048f14b9e.bundle.js:43528:172)
    at Ig (http://localhost:6006/vendors~main.b507b5bad99048f14b9e.bundle.js:43519:137)
    at bk (http://localhost:6006/vendors~main.b507b5bad99048f14b9e.bundle.js:43570:43)
    at http://localhost:6006/vendors~main.b507b5bad99048f14b9e.bundle.js:43573:301

I was following https://www.gatsbyjs.org/docs/visual-testing-with-storybook/ tutorial

@sidharthachatterjee
Copy link
Contributor Author

@DanailMinchev Thank you so much for reporting! Can you open an issue please with a link to your reproduction? I’ll take a look tomorrow!

@DanailMinchev
Copy link
Contributor

@sidharthachatterjee Thank you for your quick reply, apologies, I think another dependency breaks the Storybook integration, I will have a look and let you know in a separate issue, thank you

@DanailMinchev
Copy link
Contributor

Looks like it is related to @babel/core which is upgraded from 7.10.4 to 7.10.5, will create a separate issue

@herecydev
Copy link
Contributor

Hey @sidharthachatterjee, I'm also seeing the above issue (I tried isolating just a gatsby upgrade and it's still problematic). I can't see any network calls being made to the static folder which I assume is now necessary based on the changes you've made

I think that this change has also busted conditional page builds for static queries. Previously, as this was embedded into js, a change to a static query result would change the webpack compilation hash, and you get a full rebuild (not ideal but at least the content isn't stale).

Now any change in the static query result is not affecting the pages (conditional page builds is only interested in the page-data). This effectively has broken static queries with condtional page builds

@sidharthachatterjee
Copy link
Contributor Author

@herecydev Yup, conditional page builds only handles page data changes and will need more work to support this. We didn't block on it since it's experimental but should fix that soon (can you open an issue for this please?)

I'm also seeing the above issue (I tried isolating just a gatsby upgrade and it's still problematic). I can't see any network calls being made to the static folder which I assume is now necessary based on the changes you've made

Regarding this, are static query results not loading for your site? They are preloaded and heavily cached globally per site so it might be easy to miss. In case you have a reproduction, could you please open an issue? Thanks!

@herecydev
Copy link
Contributor

Yep I'll create a ticket for conditional page builds and create a simple PR for the docs (effectively warning people against using static query for now)

I'll try to put a repro together later, what I can say that might help is that none of the page-data has any staticQueryHashes in there (it's all empty arrays). I've glanced through your PR and that seems to be the "glue" that's necessary for the data to be fetched.

@sidharthachatterjee
Copy link
Contributor Author

Yep I'll create a ticket for conditional page builds and create a simple PR for the docs (effectively warning people against using static query for now)

That's super helpful! Thank you so much 🙌

I've glanced through your PR and that seems to be the "glue" that's necessary for the data to be fetched.

You're correct. If it's empty for pages that does include static queries, that's a bug.

@herecydev
Copy link
Contributor

Yep, all empty (tried pages created from "pages" and templated via createPages).

@sidharthachatterjee
Copy link
Contributor Author

@herecydev Feel free to assign me to the issue with the reproduction. I'll investigate immediately.

@herecydev
Copy link
Contributor

Thanks, it won't be for several hours unfortunately. Hopefully it's simple to repro 🤞

@herecydev
Copy link
Contributor

herecydev commented Jul 21, 2020

Actually only took a few minutes: https://github.com/herecydev/gatsby-static-query-bug

Clone, gatsby build, gatsby serve and observe the console errors. Hope that helps!

@sidharthachatterjee
Copy link
Contributor Author

@herecydev Thank you! Just tried it out. Can't reproduce. Works perfectly fine for me. Can you try running gatsby clean and try again? Also, let's move this to an issue please.

Screenshot 2020-07-21 at 6 15 35 PM

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…5723)

* Load static query results in Gatsby runtime

* Update snapshots

* Add in flight check and caching

* Revert loader.loadPageSync behaviour

* Use optional chaining

* Use memoized get across loader

* Refactor loader and clean up

* Remove console.log

* Add a separate in flight map per page

* Add back assertion for Promise in loader

* Make fetchPageDataJson a class function

* Ensure static queries work in wrapRootElement

* Disable gatsby-ssr wrapRootElement

* Fix static query result fetch calls when using __PATH_PREFIX__

* Add back gatsby-ssr

* Add preload headers for static query results

* Return undefined when page is broken in loader

* Set error state in pageDb

* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants