MONEI / Shopify-api-node

Node Shopify connector sponsored by MONEI
https://monei.com/shopify-payment-gateway/
MIT License
946 stars 278 forks source link

TypeError: Object.defineProperties called on a non-object #466

Closed ctrlaltdylan closed 3 years ago

ctrlaltdylan commented 3 years ago

I haven't started digging into the shopify-api-node/index.js:158 location yet, but just making an issue thread so when I debug we have a place to find the answer.

Mar 15 17:20:54 real-id app/web.4 TypeError: Object.defineProperties called on non-object
Mar 15 17:20:54 real-id app/web.4     at Function.defineProperties (<anonymous>)
Mar 15 17:20:54 real-id app/web.4     at /app/node_modules/shopify-api-node/index.js:158:18
Mar 15 17:20:54 real-id app/web.4     at runMicrotasks (<anonymous>)
Mar 15 17:20:54 real-id app/web.4     at processTicksAndRejections (internal/process/task_queues.js:93:5)
Mar 15 17:20:54 real-id app/web.4     at async createCheckForOrder (/app/.next/server/pages/api/orders/create-check.js:284:17)
Mar 15 17:20:54 real-id app/web.4     at async /app/.next/server/pages/api/webhooks/shop/order.js:926:14
Mar 15 17:20:54 real-id app/web.4     at async apiResolver (/app/node_modules/next/dist/next-server/server/api-utils.js:8:1)
Mar 15 17:20:54 real-id app/web.4     at async Server.handleApiRequest (/app/node_modules/next/dist/next-server/server/next-server.js:62:403)
Mar 15 17:20:54 real-id app/web.4     at async Object.fn (/app/node_modules/next/dist/next-server/server/next-server.js:54:176)
Mar 15 17:20:54 real-id app/web.4     at async Router.execute (/app/node_modules/next/dist/next-server/server/router.js:23:67)
Mar 15 17:20:54 real-id app/web.4     at async Server.run (/app/node_modules/next/dist/next-server/server/next-server.js:63:656)
Mar 15 17:20:54 real-id app/web.4     at async Server.handleRequest (/app/node_modules/next/dist/next-server/server/next-server.js:33:242)
lpinca commented 3 years ago

It means that received data is not an object but the HTTP response has a pagination Link header which is a bit weird.

lpinca commented 3 years ago

I'm closing this due to inactivity.

tderouin commented 2 years ago

I'm receiving this error. The link in the header returned looks like this:

"link": "<https://XXXX.myshopify.com/admin/variants.json?limit=50&page_info=eyJsYXN0X2lkIjozNjg0ODA0NjYzNzIyNCwibGFzdF92YWx1ZSI6IjM2ODQ4MDQ2NjM3MjI0IiwiZGlyZWN0aW9uIjoibmV4dCJ9>; rel=\"next\"",

Seems to happen when I pass null to this:

const shopify_variant = await shopify.productVariant.get(null);

a valid body is returned of variants.

LMK if you need any more info.

tderouin commented 2 years ago

It appears that when someone places an order, and the product variant is deleted, the line_item has a null variant_id, which is where I'm running into this problem:

        for (const line_item of entry.shopify_order.line_items) {
          const variant_id = line_item.variant_id;
          if (!variant_id) {
            console.log(`No variant id for line item ` + JSON.stringify(line_item, null, ' '));
            continue;
          }
          const shopify_variant = await shopify.productVariant.get(variant_id);
          ... more stuff
        } 
lpinca commented 2 years ago

Why is shopify.productVariant.get() called with null? It seems a programmer error to me. The !variant_id check in your code should prevent this, no?

tderouin commented 2 years ago

Shopify is returning a valid response, seems like an error in your code, no?

lpinca commented 2 years ago

The problem is that shopify.productVariant.get() is called with null. The id parameter must be a number as per documentation.

Ifshopify.productVariant.get() is called with null, then the library behavior is undefined.

What happens is that the result is not a single product variant but a list of variants. Shopify returns this list, then the library tries to read the single variant here https://github.com/MONEI/Shopify-api-node/blob/3.8.1/index.js#L164 where key is 'variant' because you requested a single one. There is no such key because the actual key is 'variants'. Then the error is thrown.

I think this is expected because null is not a valid parameter for shopify.productVariant.get().

tderouin commented 2 years ago

Whatever.

lpinca commented 2 years ago

The "fix" would be immediately throwing an error if the id argument of shopify.productVariant.get() is not a number because that is a programmer error.

panoply commented 2 years ago

Chiming in here. I experience this error regularly and am yet to actually figure out what is causing it. Likely the architecture and general approaches taken across the code base. As mentioned in previous issues, the dynamic requires are a frequent issue. I use this lib in a lambda, the strangest thing is that some deploys work no problems at all but the vast majority of the time this issue occurs. The only way around it is to continually execute the deploys until it works. Heavy days it is.

DeprecationWarning: OutgoingMessage.prototype._headers is deprecated

5:05:56 AM: 8096fde8 ERROR  TypeError: Object.defineProperties called on non-object
    at Function.defineProperties (<anonymous>)
    at /var/task/packages/api/node_modules/shopify-api-node/index.js:176:18

I would move libs but Shopify does a horrible job at this aspect. I would fork and overhaul but I just don't have the time. If anyone has experienced the same, please shed some light.

lpinca commented 2 years ago

@panoply this cannot be caused by dynamic requires. Make sure you are actually calling the methods with the correct arguments. For example, shopify.<resource>.get(null) can cause this error.

panoply commented 2 years ago

I am calling the correct methods. Dynamic requires are just a side issue so-to-speak because they prevent the library from being treeshaked and included in builds, as such one could negate such errors if it did not have to be treated as an external because bundlers could align.

It is import to note that this only occurs on occasions and seems be directly related to the Object.defineProperties called on non-object error and it being thrown within a Lambda env (Netlify specifically) which from my knowledge leverages ESBuild to compile the serverless functions. Something happens internally that is causing this issue to be thrown but again, this only occurs occasionally, adjusting the logic is likely to suffice.

It is a strange issue to be honest, very little contextual information to go from.

lpinca commented 2 years ago

I am calling the correct methods

Yes, but one method might return something that is not what you expect, like lineItem.variant_id being null as per https://github.com/MONEI/Shopify-api-node/issues/466#issuecomment-995041008. If you blindly pass that value to another method call, then the issue might occur. The reason is written in https://github.com/MONEI/Shopify-api-node/issues/466#issuecomment-995112309.

panoply commented 2 years ago

I hear you, but it is just not the case, given that it is operational and works without issue. The error is thrown on occasions and the values passed to methods are validated, nothing is passed blindly on my end. I encounter the error when re-deploying to the environment in which I am leveraging it (AWS Lambda via Netlify).

The way Netlify processes Lambdas is with zip-it-and-ship-it which will process dependencies using zisi, esbuild or esbuild_zisi. My hypothesis is when these bundlers are processing files the library code is augmented. Again, I have little context to go from here given that this will only occur in deployment (production) and not in development but it is a real hassle to say the least. I will do a little more digging to see that I can uncover but in any sense is rethinking how the library handles this out of a realm of possibilities?

I understand that this is OS tooling and I thank you for your efforts and the time you invest into maintaining the project. Refactoring some areas might be worth consideration, no?

lpinca commented 2 years ago

Refactoring some areas might be worth consideration, no?

If there is evidence that they are the cause of issues, then yes. Otherwise no, refactoring for the sake of refactoring does not make sense.

codeyourwayup commented 2 years ago

some node version won't work! try v12.22.3