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

Break variants into separate operation #32027

merged 10 commits into from Jun 23, 2021

Conversation

sslotsky
Copy link
Contributor

@sslotsky sslotsky commented Jun 22, 2021

Description

Breaking product variants into a separate operation so that presentment prices can be added.

Shopify's bulk API has a limit on the number of connections that can be requested in a single query, so in order to support presentmentPrices requested by community members, we need to pull variants out into a separate operation.

Documentation

Related Issues

https://app.clubhouse.io/gatsbyjs/story/32892/move-product-variants-into-a-separate-operation
https://github.com/gatsbyjs/gatsby-source-shopify/issues/161

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 22, 2021
Copy link
Contributor Author

@sslotsky sslotsky left a comment

Choose a reason for hiding this comment

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

We have a problem with the test configuration where Jest thinks that fixtures.ts is a test file and complains that there are no test cases in it. We've tried to modify the jest.config.js at the root but we can't get it to ignore this file. We've tried adding the exact path, e.g.

  testPathIgnorePatterns: [
    `<rootDir>/packages/gatsby-src-shopify/__tests__/fixtures.ts`,

And we've also tried moving fixtures.ts to __tests__/fixtures/index.ts to try and match the __tests__/fixtures pattern that's already there. @wardpeet any ideas on this one?

@sslotsky
Copy link
Contributor Author

We have a problem with the test configuration

...which seems to only be an issue locally, by the way.

@sslotsky sslotsky marked this pull request as ready for review June 23, 2021 18:19
@sslotsky sslotsky requested review from DanielSLew and a team June 23, 2021 20:05
Comment on lines +8 to +12
const objectsToBuild = objects.filter(obj => {
const [, remoteType] = obj.id.match(idPattern) || []

return remoteType !== `Product`
})
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.

Copy link
Contributor

@DanielSLew DanielSLew 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! Just had one question to clarify something.

@sslotsky sslotsky merged commit 069cb53 into master Jun 23, 2021
@sslotsky sslotsky deleted the sslotsky/ch32892 branch June 23, 2021 20:26
@LekoArts LekoArts removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 28, 2021
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.

None yet

3 participants