-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[v2] Implement RFC #2 removing special layout component #4887
Conversation
Sometime in v2 cycle we need to move all query running to on-demand and only watch components that are active on the page, etc. Also need to somehow make createPages not so centralized. This is so we can scale gatsby develop to larger and large sites. |
It'd also really speed up hot reloading data during development. Larger sites can accumulate lots of queries that are affected by a single data change. E.g. changing a markdown file in gatsbyjs.org can trigger running ~20 queries — only one of which is relevant to what the developer is looking at. |
Works by creating a hash of the query in the component using babel and then extracting that, running it like page queries, and then pushing the result to the browser where it can be pulled in off the context to be displayed. TODO still is implement this for production. In production, we'll add an import for the query result into the component with the <StaticQuery> and then pass that imported JSON into <StaticQuery> which will then pass it right back into the render prop. Yeah for indirection!
Upgraded my blog to use this PR. Got rid of some duplicate data loading and moved those to component queries 🎉 https://github.com/KyleAMathews/blog/pull/21/files |
Some new patterns which emerged as part of the upgrade of my blog
|
const query = text | ||
|
||
// Replace the query with the hash of the query. | ||
path2.replaceWith(t.StringLiteral(queryHash)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not replace with a require
to the data itself, you could avoid the context bit in that case maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we moved to loading query results over websockets in #4555 as it's way way faster than relying on webpack to hot reload data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oo right nice, i was trying to think of how you could avoid the hash here hmm
Updated gatsbyjs.org — this didn't require using |
} | ||
}) | ||
} | ||
const definitions = [...gqlAst.definitions].map(d => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying this out, sometimes gqlAst.definitions
isn't defined so this errors with a There was a problem parsing the GraphQL query in file
message. Should this stuff live inside the if (gqlAst) {}
block?
TaggedTemplateExpression(path) { | ||
if ( | ||
(`descendant of query`, | ||
path?.parentPath?.parentPath?.node?.name?.name !== `query`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the export const query = ...
case yeah? maybe path.findParent(p => p.isVariableDeclarator)?.id.name
is a better option here (and below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh findParent
exists? 😅I just fumbled my way through the whole babel writing — I'll refactor this then with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i just found out about it reading the emotion plugin recently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, tried this and couldn't get it working after 5-10 minutes of trying. I never know what babel types to use exactly nor how to call them. We're trying to identify if the tagged template is inside the StaticQuery component and query prop. If you want to take a crack at this sometime, that'd be great.
<StaticQuery query={graphql`the_query`} />
return | ||
} | ||
if ( | ||
path.parentPath?.parentPath?.parentPath?.node?.name?.name !== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work in the case where someone is or isn't using JSX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No — that'd be great to support though too.
This proved fairly difficult — Gatsbygram's modal architecture is always a good canary in the coalmine for ensuring Gatsby's architecture is flexible. It ended up being quite elegant though: * Used <StaticQuery> to load the image metadata into the Modal component so it could loop through the images. * Exposed <PageRenderer> through the Gatsby package so that the layout could render the index page underneath the modal when it is showing * Use <React.Fragment> around the Index/Modal components so that the Index page component didn't need rendered to the DOM again * To detect if this was the initial render, I implemented onInitialClientRender and set a global on the window so that the post template component would know *not* to use the modal if the user went directly to a post page.
Upgrade GatsbygramThis proved fairly difficult — Gatsbygram's modal architecture is always
|
Ok, this is feeling pretty solid! Going to merge it in and try to get master merged into v2 as well. |
} | ||
|
||
render() { | ||
const pluginResponses = apiRunner(`replaceComponentRenderer`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we lost this api...
gatsbyjs#4887) [v2] Implement RFC gatsbyjs#2 removing special layout component
RFC https://github.com/gatsbyjs/rfcs/blob/remove-layouts/text/0000-remove-special-layout-components.md
TODOs
<StaticQuery>
for development<StaticQuery>
for productioncomponent-renderer
topage-renderer
?Signed-off-by: Kyle Mathews [email protected]