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

Break variants into separate operation #32027

Merged
merged 10 commits into from
Jun 23, 2021
Merged
9 changes: 9 additions & 0 deletions packages/gatsby-source-shopify/.babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"presets": [["babel-preset-gatsby-package"]],
"overrides": [
{
"test": [],
"presets": [["babel-preset-gatsby-package", { "browser": true, "esm": true }]]
}
]
}
9 changes: 6 additions & 3 deletions packages/gatsby-source-shopify/__tests__/fixtures.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import {
GraphQLContext,
GraphQLRequest,
ResponseResolver,
graphql,
ResponseComposition,
GraphQLHandler,
MockedResponse,
} from "msw"

Expand All @@ -25,7 +26,7 @@ export function resolve<T>(data: T): Resolver<T> {

export function currentBulkOperation(
status: BulkOperationStatus
): Record<string, unknown> {
): CurrentBulkOperationResponse {
return {
currentBulkOperation: {
id: ``,
Expand All @@ -38,7 +39,9 @@ type BulkNodeOverrides = {
[key in keyof BulkOperationNode]?: BulkOperationNode[key]
}

export const startOperation = (overrides: BulkNodeOverrides = {}): any => {
export const startOperation = (
overrides: BulkNodeOverrides = {}
): GraphQLHandler<GraphQLRequest<BulkOperationRunQueryResponse>> => {
const { id = `12345` } = overrides

return graphql.mutation<BulkOperationRunQueryResponse>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {

const server = setupServer()

// @ts-ignore
// @ts-ignore because these types will never match
global.setTimeout = (fn: Promise<void>): Promise<void> => fn()

jest.mock(`gatsby-source-filesystem`, () => {
Expand Down Expand Up @@ -69,7 +69,9 @@ describe(`The collections operation`, () => {
server.use(
graphql.query<CurrentBulkOperationResponse>(
`OPERATION_STATUS`,
resolveOnce(currentBulkOperation(`COMPLETED`))
resolveOnce<CurrentBulkOperationResponse>(
currentBulkOperation(`COMPLETED`)
)
),
startOperation(),
graphql.query<{ node: BulkOperationNode }>(
Expand Down Expand Up @@ -709,14 +711,6 @@ describe(`The incremental products processor`, () => {
{
id: firstProductId,
},
{
id: firstVariantId,
__parentId: firstProductId,
},
{
id: firstMetadataId,
__parentId: firstVariantId,
},
{
id: firstImageId,
__parentId: firstProductId,
Expand Down Expand Up @@ -836,7 +830,7 @@ describe(`The incremental products processor`, () => {

await sourceFromOperation(operations.incrementalProducts(new Date()))

expect(createNode).toHaveBeenCalledTimes(4)
expect(createNode).toHaveBeenCalledTimes(2)
expect(deleteNode).toHaveBeenCalledTimes(6)

expect(deleteNode).toHaveBeenCalledWith(
Expand Down Expand Up @@ -888,20 +882,6 @@ describe(`The incremental products processor`, () => {
})
)

expect(createNode).toHaveBeenCalledWith(
expect.objectContaining({
id: firstMetadataId,
productVariantId: firstVariantId,
})
)

expect(createNode).toHaveBeenCalledWith(
expect.objectContaining({
id: firstVariantId,
productId: firstProductId,
})
)

expect(createNode).toHaveBeenCalledWith(
expect.objectContaining({
id: firstProductId,
Expand Down
10 changes: 8 additions & 2 deletions packages/gatsby-source-shopify/src/gatsby-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,15 @@ async function sourceAllNodes(
): Promise<void> {
const {
createProductsOperation,
createProductVariantsOperation,
createOrdersOperation,
createCollectionsOperation,
finishLastOperation,
completedOperation,
cancelOperationInProgress,
} = createOperations(pluginOptions, gatsbyApi)

const operations = [createProductsOperation]
const operations = [createProductsOperation, createProductVariantsOperation]
if (pluginOptions.shopifyConnections?.includes(`orders`)) {
operations.push(createOrdersOperation)
}
Expand Down Expand Up @@ -106,6 +107,7 @@ async function sourceChangedNodes(
): Promise<void> {
const {
incrementalProducts,
incrementalProductVariants,
incrementalOrders,
incrementalCollections,
finishLastOperation,
Expand All @@ -125,7 +127,11 @@ async function sourceChangedNodes(
.forEach(node => gatsbyApi.actions.touchNode(node))
}

const operations = [incrementalProducts(lastBuildTime)]
const operations = [
incrementalProducts(lastBuildTime),
incrementalProductVariants(lastBuildTime),
]

if (pluginOptions.shopifyConnections?.includes(`orders`)) {
operations.push(incrementalOrders(lastBuildTime))
}
Expand Down
2 changes: 1 addition & 1 deletion packages/gatsby-source-shopify/src/node-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ export function nodeBuilder(
pluginOptions: ShopifyPluginOptions
): NodeBuilder {
return {
async buildNode(result: BulkResult): Promise<any> {
async buildNode(result: BulkResult): Promise<NodeInput> {
if (!pattern.test(result.id)) {
throw new Error(
`Expected an ID in the format gid://shopify/<typename>/<id>`
Expand Down
18 changes: 18 additions & 0 deletions packages/gatsby-source-shopify/src/operations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ import { NodeInput, SourceNodesArgs } from "gatsby"
import { shiftLeft } from "shift-left"
import { createClient } from "./client"
import { ProductsQuery } from "./query-builders/products-query"
import { ProductVariantsQuery } from "./query-builders/product-variants-query"
import { CollectionsQuery } from "./query-builders/collections-query"
import { OrdersQuery } from "./query-builders/orders-query"
import {
collectionsProcessor,
incrementalProductsProcessor,
productVariantsProcessor,
} from "./processors"
import { OperationError } from "./errors"

Expand All @@ -29,9 +31,11 @@ export interface IShopifyBulkOperation {

interface IOperations {
incrementalProducts: (date: Date) => IShopifyBulkOperation
incrementalProductVariants: (date: Date) => IShopifyBulkOperation
incrementalOrders: (date: Date) => IShopifyBulkOperation
incrementalCollections: (date: Date) => IShopifyBulkOperation
createProductsOperation: IShopifyBulkOperation
createProductVariantsOperation: IShopifyBulkOperation
createOrdersOperation: IShopifyBulkOperation
createCollectionsOperation: IShopifyBulkOperation
cancelOperationInProgress: () => Promise<void>
Expand Down Expand Up @@ -216,6 +220,14 @@ export function createOperations(
)
},

incrementalProductVariants(date: Date): IShopifyBulkOperation {
return createOperation(
new ProductVariantsQuery(options).query(date),
`INCREMENTAL_PRODUCT_VARIANTS`,
productVariantsProcessor
)
},

incrementalOrders(date: Date): IShopifyBulkOperation {
return createOperation(
new OrdersQuery(options).query(date),
Expand All @@ -236,6 +248,12 @@ export function createOperations(
`PRODUCTS`
),

createProductVariantsOperation: createOperation(
new ProductVariantsQuery(options).query(),
`PRODUCT_VARIANTS`,
productVariantsProcessor
),

createOrdersOperation: createOperation(
new OrdersQuery(options).query(),
`ORDERS`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ export function incrementalProductsProcessor(
return remoteType === `Product`
})

const nodeIds = products.map(product =>
const variants = objects.filter(obj => {
const [, remoteType] = obj.id.match(idPattern) || []

return remoteType === `ProductVariant`
})

const productNodeIds = products.map(product =>
createNodeId(product.id, gatsbyApi, pluginOptions)
)

Expand All @@ -26,15 +32,29 @@ export function incrementalProductsProcessor(
*/
const variantsToDelete = gatsbyApi
.getNodesByType(`${typePrefix}ShopifyProductVariant`)
.filter(node => nodeIds.includes(node.productId as string))
.filter(node => {
const index = productNodeIds.indexOf(node.productId as string)
if (index >= 0) {
const product = products[index]
const variantIds = variants
.filter(v => v.__parentId === product.id)
.map(v => createNodeId(v.id, gatsbyApi, pluginOptions))

if (!variantIds.includes(node.id)) {
return true
}
}

return false
})

variantsToDelete.forEach(variant => {
gatsbyApi.actions.deleteNode(variant)
})

const imagesToDelete = gatsbyApi
.getNodesByType(`${typePrefix}ShopifyProductImage`)
.filter(node => nodeIds.includes(node.productId as string))
.filter(node => productNodeIds.includes(node.productId as string))

imagesToDelete.forEach(image => {
gatsbyApi.actions.deleteNode(image)
Expand All @@ -54,5 +74,11 @@ export function incrementalProductsProcessor(
}
})

return objects.map(builder.buildNode)
const objectsToBuild = objects.filter(obj => {
const [, remoteType] = obj.id.match(idPattern) || []

return remoteType !== `ProductVariant`
})

return objectsToBuild.map(builder.buildNode)
}
1 change: 1 addition & 0 deletions packages/gatsby-source-shopify/src/processors/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from "./collections"
export * from "./incremental-products"
export * from "./product-variants"
26 changes: 26 additions & 0 deletions packages/gatsby-source-shopify/src/processors/product-variants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { NodeInput } from "gatsby"
import { pattern as idPattern } from "../node-builder"

export function productVariantsProcessor(
objects: BulkResults,
builder: NodeBuilder
): Array<Promise<NodeInput>> {
const objectsToBuild = objects.filter(obj => {
const [, remoteType] = obj.id.match(idPattern) || []

return remoteType !== `Product`
})
Comment on lines +8 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to check if it's not a Product because we can't query for product variants directly and the new Productvariant query query's for products and product variants?

Copy link
Contributor Author

@sslotsky sslotsky Jun 23, 2021

Choose a reason for hiding this comment

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

No and yes.

We can query for variants at the top level, but I found that it didn't work quite right for incremental builds. It seems that when you add a variant, querying for products that are updated_at >= last_build_time pulls them back, but when you query for variants there's no created_at filter, and updated_at >= last_build_time doesn't pull it back.

So because of that, I wrapped our new variants query in a products query that takes the same filters as the main products query does, and now we get back all the correct records. But as a result, we get all these Product records in the results that we don't need because we already created nodes for them.

I suspect that merging this PR might mean that cold builds are a little bit slower. Hopefully not much. But without doing this, we won't be able to add presentmentPrices without removing something else, so I think we have to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, that's kind of what I figured the tradeoff would be, thanks for the explanation.


/**
* We will need to attach presentmentPrices here as a simple array.
* To achieve that, we could go through the results backwards and
* save the ProductVariantPricePair records to a map that's keyed
* by the variant ID, which can be obtained by reading the __parentId
* field of the ProductVariantPricePair record.
*
* We do similar processing to collect the product IDs for a collection,
* so please see the processors/collections.ts for reference.
*/

return objectsToBuild.map(builder.buildNode)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import { BulkQuery } from "./bulk-query"

export class ProductVariantsQuery extends BulkQuery {
query(date?: Date): string {
const publishedStatus = this.pluginOptions.salesChannel
? encodeURIComponent(`${this.pluginOptions.salesChannel}=visible`)
: `published`

const filters = [`status:active`, `published_status:${publishedStatus}`]
if (date) {
const isoDate = date.toISOString()
filters.push(`created_at:>='${isoDate}' OR updated_at:>='${isoDate}'`)
}

const ProductVariantSortKey = `POSITION`

const queryString = filters.map(f => `(${f})`).join(` AND `)

const query = `
{
products(query: "${queryString}") {
edges {
node {
id
variants(sortKey: ${ProductVariantSortKey}) {
edges {
node {
availableForSale
barcode
compareAtPrice
createdAt
displayName
id
image {
id
altText
height
width
originalSrc
transformedSrc
}
inventoryPolicy
inventoryQuantity
legacyResourceId
position
price
selectedOptions {
name
value
}
sellingPlanGroupCount
sku
storefrontId
taxCode
taxable
title
updatedAt
weight
weightUnit
metafields {
edges {
node {
createdAt
description
id
key
legacyResourceId
namespace
ownerType
updatedAt
value
valueType
}
}
}
}
}
}
}
}
}
}
`

return this.bulkOperationQuery(query)
}
}
Loading