-
Notifications
You must be signed in to change notification settings - Fork 57
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: product page incremental builds #793
Conversation
✔️ Deploy Preview for storeui ready! 🔨 Explore the source changes: bbec9b9 🔍 Inspect the deploy log: https://app.netlify.com/sites/storeui/deploys/60db3100e69a950008ae9c1d 😎 Browse the preview: https://deploy-preview-793--storeui.netlify.app |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit bbec9b9:
|
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.
Nice job! I left some comments.
'Department', | ||
'Category', | ||
'Brand', | ||
'SubCategory', | ||
] | ||
|
||
const readFile = promisify(readFileAsync) |
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.
Can you explain why you need to use the promisify? I was reading the node doc and saw that the readFile already return a promise.
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.
nice suggestion. Then I'd need to import from fs/promises
instead of just fs
. However fs/promises is available in node14 only. Do you think we can increase our node dependency?
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.
Hmm, good point. I think we can increase our node to v14, because we are in alpha, and after alpha will be more difficult to change. I'm only seeing good points bumping the node dependency, what do you think about it?
* Source products | ||
*/ | ||
promisses.push(async () => { | ||
// Step1. Set up remote schema: |
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.
What do you think about create functions for each step? It may improve maintainability and readability. Also, you can create documentation for each step, for others that don't have too much context can understand too.
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'm not sure, like this it's easy to follow the tutorial and understand what's happening
https://github.com/gatsbyjs/gatsby-graphql-toolkit#how-it-works
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.
Yeah, I can understand, and your comment directs well the reader to the specific documentation. Good work commenting.
@@ -0,0 +1,45 @@ | |||
import { graphql } from 'gatsby' |
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.
Can you add a suggestive name for this file? What does this p
mean?
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.
not really, because this is the path. Changing the name of this file changes the route generated. So to have a product in the path /<slug>/p
we need to have a file named p.tsx
in our filesystem
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.
Yeah, i forget that gatsby uses the file's path.
export const query = graphql` | ||
query ServerProductPageQuery($id: String!) { | ||
product: storeProduct(id: { eq: $id }) { | ||
...ProductDetailsTemplate_product | ||
...StructuredProductFragment_product | ||
titleTag | ||
metaTagDescription | ||
id: productId | ||
description | ||
categoryTree { | ||
name | ||
href | ||
} | ||
} | ||
} | ||
` |
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.
Just a doubt, why our queries can't be written inside .gql
or .graphql
files?
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.
It's because on how Gatsby query processing works
This PR is breaking on btglobal.store at the product page. |
This is because of an unrelated issue. More info at: https://vtex.slack.com/archives/C92590P39/p1624912880182900 |
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.
Nice, seems good! Nice job
'Department', | ||
'Category', | ||
'Brand', | ||
'SubCategory', | ||
] | ||
|
||
const readFile = promisify(readFileAsync) |
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.
Hmm, good point. I think we can increase our node to v14, because we are in alpha, and after alpha will be more difficult to change. I'm only seeing good points bumping the node dependency, what do you think about it?
* Source products | ||
*/ | ||
promisses.push(async () => { | ||
// Step1. Set up remote schema: |
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.
Yeah, I can understand, and your comment directs well the reader to the specific documentation. Good work commenting.
What's the purpose of this pull request?
As it's described in here, using the addThirdPartySchema API goes against the Gatsby architecture and causes many problems when using Gatsby's incremental builds. This PR uses gatsby-graphql-toolkit for sourcing data and uses the declarative Gatsby File System API for declaratively create product pages. Also, from now on, we will use Gatsby's cache for not having to fetch all products every time. If you want to refresh the products, you will have to clear the cache Gatsby's cache with
These optimizations made me change a little bit the GraphQL schema we were using. Now, you can query for product during build using the APIs
query { storeProduct }
andquery { allStoreProducts }
and the product is behind the new StoreProduct type.How to test it?
https://github.com/vtex-sites/btglobal.store/pull/663
https://github.com/vtex-sites/marinbrasil.store/pull/536
https://github.com/vtex-sites/storecomponents.store/pull/1001
https://github.com/vtex-sites/carrefourbrfood.store/pull/859
Results
The table below compares the build times, in seconds, without any cache, with cache and the current build times (controls).