pasdo501 / gatsby-source-woocommerce

Gatsy Source Plugin for WooCommerce
34 stars 14 forks source link

Conflicting GraphQL types #22

Closed shendriksza closed 2 years ago

shendriksza commented 4 years ago

Compiling gives me the following warning:

warn There are conflicting field types in your data.

If you have explicitly defined a type for those fields, you can safely ignore this warning message.
Otherwise, Gatsby will omit those fields from the GraphQL schema.

If you know all field types in advance, the best strategy is to explicitly define them with the `createTypes` action, and skip inference with the `@dontInfer` directive.
See https://www.gatsbyjs.org/docs/actions/#createTypes
wcProducts.price:
 - type: number
   value: 0
 - type: string
   value: '10694.783'
wcProducts.regular_price:
 - type: number
   value: 0
 - type: string
   value: ''
wcProducts.product_variations.manage_stock:
 - type: boolean
   value: false
 - type: string
   value: 'parent'

These fields being removed from the GraphQL schema is obviously a problem 😅

I'm not sure how field types are inferred, and apart from hardcoding the types in the createTypes action I'm not sure what the solution would be. The issue with manage_stock is caused by the way we set up product variations to inherit some properties from the parent, but the prices being the wrong type doesn't make sense to me (unless the number type does not support float?). Any advice will be appreciated!

pasdo501 commented 4 years ago

Hey mate,

That whole price thing is quite strange, yes. I think the number type is referring to the JavaScript number type, i.e. double - so that shouldn't be an issue.

This description seems to suggest that field types are inferred by merging all the possible values it gets back, although why this leads to different types in this case beats me (but a. that article is apparently out of date and b. I'm probably missing something).

Adding the types to createTypes would be a valid solution, although it wouldn't help with the manage_stock issue, since that's more of a custom issue I would think. In the mean time, since that issue cropped up after changes to the Woocommerce data (I assume it was working before?), could you maybe try running gatsby clean (unless you've already tried that) before trying to query the data again? I wonder if it might have some type definitions left in the cache that are now conflicting with the new data (I don't know if types are actually cached in any way at all unless specified - so this may be a silly suggestion - I just remember that I was running into issues with field names early on in the development of this plugin that seemed cache related).

shendriksza commented 4 years ago

Hi pasdo

I've managed to reverse the problem. The issue happened when we added a specific test product with a price of zero. When I do a GET request to the WordPress site I can see that a price of zero is returned as a number, while a non-zero price is returned as a string.

This obviously confuses Gatsby because as you said, types are inferred by all values. The field is then removed from the schema which means our GraphQL query fails which means our site cannot build... In the meantime we worked around the issue, but it might be a good idea to somehow always type-cast the prices to string before the types are created?

The manage_stock boolean has the same problem, if a product variation's stock management is set to follow the parent instead of being handled on its own, they mark it with a string (which is dumb, WP should really not be mixing types like this). We don't use this field so it's not included in our GraphQL queries, but it's something to keep in mind as well.

And to cover all bases, I did indeed try running Gatsby clean and it had no effect. Your hunch is correct, GraphQL types definitely do get cached

pasdo501 commented 4 years ago

Hey there,

Is there anything else special about the product, or is it just a price of zero? I tried adding such a product, and it seems to happily convert to string ("0") for me. Since it's warning you about incorrect types for price though, would you be able to test if 4297d055b679e17f875184a500afe193d58cf24e fixes that issue (with the string / number type for the price), or if it throws an error? Basically to see if defining the type makes it convert those values it can, or if it's seen as an error instead. Although if the warning about the type conflict remains (for the price), that's not really a solution, and would have to go down the path of defining all field types and then flag them with dontInfer.

The manage_stock thing seems more annoying, I'm not really sure how to handle dealing with that without it affecting people already using the plugin? Otherwise, I suppose during the node processing, but before creation, could deal with turning them all into strings (presumably - to handle the more than 2 states it can be in). Any ideas?

shendriksza commented 4 years ago

Hi again

Yes you were correct. I tested some more and realised it wasn't the variations causing the issues.

We use an official WooCommerce extension called Product Bundles that allows us to bundle products together into one, and it is the one causing the price type issue. It officially supports the existing API and it seems to work great using your plugin, but because it simply adds to the /products endpoint there is no easy way to prevent it from polluting the structure and breaking our types.

I tried your build but it didn't seem to work, I'm not sure why. But I'll be opening a ticket with WooCommerce support so they can fix it in the next release as it's definitely a bug.

The manage_stock type is a native problem and I don't see how it can be fixed without affecting existing users. But most people will probably either manage stock for all products, or none, in which case it shouldn't be an issue for them and there are ways to work around it with eg. custom attributes