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

BREAKING CHANGE(gatsby-source-shopify): Rewrite for media, presentment, schema, etc. #34049

Merged
merged 97 commits into from
Mar 24, 2022
Merged

BREAKING CHANGE(gatsby-source-shopify): Rewrite for media, presentment, schema, etc. #34049

merged 97 commits into from
Mar 24, 2022

Conversation

byronlanehill
Copy link
Contributor

@byronlanehill byronlanehill commented Nov 22, 2021

Description

This Pull Request is a nearly complete rewrite of the gatsby-source-shopify plugin that fixes quite a few of the issues that I and others have been having with the plugin for some time.

  1. Product Media - Previously you could only get product images through the product.images field. THis made it impossible to get product videos or 3D Models. This PR fixes that issue by replacing product.images with product.media.

  2. Correct Reference Order - Previously, because of the way that we were referencing child fields we were unable to guarantee the correct order of children (variants, images, etc). We fixed this by fetching the order from Shopify and converting the order to an array of foreign-key references.

  3. Single Metafield type - To keep in line with the shopify Admin API Schema, we switched to having a single Metafield type as opposed to multiple "{parent}Metafield" types.

  4. Added presentmentPrices - Previously, because of the way that we were parsing the results return by the Shopify Bulk API we were unable to fetch presentment prices. With the new method of parsing results we were able to, so that field is now available on product variants.

  5. Explicit TypeDefs - We decided to add the complete schema to this plugin and disable schema inference, which allows us to account for the possibility that there are no products in Shopify or that a field is null without breaking every page on the site due to schema inference.

  6. Match API Schema - The Gatsby schema for the plugin matches the Shopify Admin API schema ~95%. This allows us to refer to the Shopify documentation for the majority of fields rather than having to create separate documentation to consume the same data.

Outstanding Issues

  1. Update Documentation, specifically for querying for media.
  2. Create a migration guide for the new major version.

Documentation

Because this is a Draft PR and is subject to change, the documentation has not been updated as of 2021-11-22.

Related Issues

Fixes #29030
Fixes #32090
Fixes #32832
Fixes #32097

@jackonawalk
Copy link
Contributor

@byronlanehill would you be able to split this into multiple PRs? That'll make it easier to review and rollback if there are any regressions (since this is such a significant upgrade). Also curious, what's been your experience using the admin bulk operation so far?

@byronlanehill
Copy link
Contributor Author

@jacksellwood Unfortunately I don't think that would be feasible. The vast majority of the code changes were required to complete the original goal of having a working product.media field.

The big code changes are:

  1. Changing how bulk results are processed - Previously we were referencing fields by the parent ID on the child. ie: Find all images with a parent ID of XXXX and add them to the products images field. Because Media had 4 different object types (ExternalVideo, MediaImage, Model3d, Video) it would never have been able to respect the original order of the media field.

  2. Adding explicit schema - This wasn't necessarily required, but because we completely changed the schema it made sense to finish the job and explicitly type it. There is a fair amount of lines regarding this but it is actually a fairly small change and extremely easy to test.

In theory, it would be possible to remove some of the code quality changes (moving the rest-client and graphql-client into a single file, consolidating/renaming the typescript types, moving the Gatsby Node APIs into their own files, creating a helper file).

Having said that, If you disagree or see something that you think could be split away easily that would make reviewing easier I am all ears!

What I think might be the best course of action would be to treat this as a new plugin, in terms of code review and testing, just because of how many changes there are and how the schema has changed as a result.

Looking forward to your feedback!

@byronlanehill
Copy link
Contributor Author

Overall, the Bulk Admin API is great. There are a few small issues with it, but it is working much better than my client's previous implementations of using a combination of the REST API and the GraphQL API.

miraclecra
miraclecra previously approved these changes Nov 23, 2021
…gatsbyImageData / localFile field types optional to account for Sharp being unable to process GIFs
@byronlanehill
Copy link
Contributor Author

@jacksellwood How do we go about moving forward with this?

@jackonawalk
Copy link
Contributor

@byronlanehill we're going to get this published as a canary on NPM so we can start testing it with customers. I'll have that ready later today!
cc @veryspry

@byronlanehill
Copy link
Contributor Author

Thanks @jacksellwood! I've never had anything published to NPM, if I make an update to my PR will the canary update as well? Otherwise would it be worthwhile for me to do more in depth testing before we publish?

@veryspry
Copy link
Contributor

Hey @byronlanehill! I published a canary of this package to NPM under: 6.2.0-alpha-atomic-shopify-source.16

To answer your questions:

  • no, the canary will not update automatically if you update the PR. We would have to publish the changes again for you.
  • The canary is opt in and won't get auto installed due to semver rules. Someone would have to explicitly put the canary version in their package.json. No need to do more testing for a canary 🙂

We are hoping to use these changes (thanks for taking the time to PR them!). Since they are quite extensive, we just want to ensure that this is going to be stable for our users. Publishing a canary is a way for us to test internally as well as give users an easy way to install and test these changes if they wish.

I'm also planning to review and test this today / tomorrow!

@byronlanehill
Copy link
Contributor Author

Perfect, thanks a million! I will start on testing some production builds of my clients sites with the canary tomorrow morning

@veryspry
Copy link
Contributor

veryspry commented Dec 1, 2021

Hey @byronlanehill! I did some testing with this and it seems to be looking pretty good so far. I do have a couple clarifying questions for you though.

  • Why did you ask about exporting types from get-shopify-image? If it is to maintain backwards compatibility, then that is already broken because of the images → media change. (i.e. this will likely need to be a new major release). If this is the case we probably don't need to worry about exporting from there too
  • Did the media order thing get solved that you were mentioning in this comment?

Match API Schema - The Gatsby schema for the plugin matches the Shopify Admin API schema ~95%. This allows us to refer to the Shopify documentation for the majority of fields rather than having to create separate documentation to consume the same data.

  • What 5% is missing? Can we add that to get 100% consistency?

@LekoArts
Copy link
Contributor

btw, you can batch my suggestions in the "Files changed" tab view and commit them directly

@LekoArts
Copy link
Contributor

LekoArts commented Mar 23, 2022

Bildschirmfoto 2022-03-23 um 09 06 58

Can you please make sure to activate this in the right sidebar of this PR? I can't push to your fork.

@byronlanehill
Copy link
Contributor Author

@LekoArts this branch is held on my organizations account and thus I have to give you access to the repo by inviting you to it. I have done that, let me know if you have issues.

Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for second pair of eyes from @pieh before I'll also approve

We'll also need to follow our specific procedure for releasing a new major version!

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Let's do it

Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: source-shopify Related to the gatsby-source-shopify plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants