thetrevorharmon / gatsby-theme-shopify-manager

The easiest way to start a Shopify shop on Gatsby.
https://gatsby-theme-shopify-manager.netlify.app/
MIT License
121 stars 11 forks source link

TypeError: Cannot read property 'product' of null #67

Closed ghost closed 4 years ago

ghost commented 4 years ago

This issue relates to the use of useClientUnsafe(). The following test affirms the hook matches the client mock when the component is wrapped with the context provider:

https://github.com/thetrevorharmon/gatsby-theme-shopify-manager/blob/4d052a0799f3d163754e45fdbe6512bf553c2f4d/gatsby-theme-shopify-manager/src/hooks/__tests__/useClientUnsafe.test.ts#L6-L9

However the behavior I'm seeing does not seem match the expected result during gatsby build and the build fails unexpectedly.

Here's my config:

{
  resolve: "gatsby-theme-shopify-manager",
  options: {
    shopName: process.env.SHOPIFY_SHOP_NAME,
    accessToken: process.env.SHOPIFY_ACCESS_TOKEN,
  },
},

And an example of how I'm using the hook:

import React, { FC } from "react";
import {
  useAddItemToCart,
  useClientUnsafe,
} from "gatsby-theme-shopify-manager";

const ProductDetails: FC = () => {

  const shopifyClient = useClientUnsafe();
  const confirmAvailability = useCallback(
    (productId) => {
      shopifyClient.product
        .fetch(productId)
        .then(...);
    },
    [shopifyClient.product],
  );

  return <></>
}

I considered using optional chaining (e.g. shopifyClient?.product) but that seems as if it should be unnecessary given the unit test I referenced above. I had the same thing with useCart as well without optional chaining.

Issue does not occur during gatsby develop. Plugin version: 0.1.7. Gatsby version: 2.23.11.

Thanks in advance for your eyes.

ghost commented 4 years ago

Okay looks like client and cart are expected to be null values as implemented here:

https://github.com/thetrevorharmon/gatsby-theme-shopify-manager/blob/4d052a0799f3d163754e45fdbe6512bf553c2f4d/gatsby-theme-shopify-manager/src/Context.tsx#L10-L16

And while it would be possible to provide more ContextShape to prevent issues like the one I experienced the current approach feels the right way to go and makes no assumptions about what the SDK will return.

Closing this issue.