Gql codegen #297
Gql codegen #297
Conversation
| host | ||
| # I don't know why these two fields break the codegen tool | ||
| # embedUrl | ||
| # host |
There was a problem hiding this comment.
Super weird but I narrowed it down to these two fields that break the graphql codegen tool when they're uncommented. 🤷
| export const MEDIA_FRAGMENT = `#graphql | ||
| export const MEDIA_FRAGMENT = /* GraphQL */ ` |
There was a problem hiding this comment.
The generator doesn't work with comments? I kind of liked the #graphql thingy.
There was a problem hiding this comment.
Yeah the comment was great. Would bringing back something like graphql help avoid /* GraphQL */?
There was a problem hiding this comment.
There was a problem hiding this comment.
For visibility: #271 (comment)
In v1 we were only using AST during development, but production only worked with strings.
Can't we do something similar here? Like, when building for prod, we just generate minified strings instead.
The generated files could even live inside node_modules so they are not visible:
import {gql} from '@shopify/hydrogen-remix => gql knows about generated types in node_modules/@shopify/hydrogen-remix/generated-gql-types.ts. Something like that? 🤔
There was a problem hiding this comment.
Haha, can't decide whether to have the conversation here or there. #271 (comment)
| ${PRODUCT_CARD_FRAGMENT} | ||
| query homepageFeaturedProducts($country: CountryCode, $language: LanguageCode) | ||
| query HomepageFeatureProducts($country: CountryCode, $language: LanguageCode) |
There was a problem hiding this comment.
Should this be HomepageFeaturedProducts?
| @@ -0,0 +1,224 @@ | |||
| import * as Types from '@shopify/hydrogen-react/storefront-api-types'; | |||
There was a problem hiding this comment.
Do we need to add any type of ignore pattern to remix so it doesn't consider this a route? Maybe it already doesn't consider it a route, I'm not sure.
There was a problem hiding this comment.
Yeah, would need to do that before this PR is merged, assuming that we want to go this route. Still discussing with the team
| @@ -9,7 +9,8 @@ | |||
| "preview": "npm run build && shopify hydrogen preview", | |||
| "lint": "eslint --no-error-on-unmatched-pattern --ext .js,.ts,.jsx,.tsx .", | |||
| "format": "prettier --write --ignore-unknown .", | |||
| "typecheck": "tsc --noEmit" | |||
| "typecheck": "tsc --noEmit", | |||
| "codegen": "graphql-codegen --config codegen.ts" | |||
There was a problem hiding this comment.
What do you think about running codegen in watch mode for the dev task?
| embedUrl | ||
| host | ||
| # I don't know why these two fields break the codegen tool | ||
| # embedUrl |
There was a problem hiding this comment.
I think I figured out what's going on here. I simplified things down as much as I could, and found that this just isn't a thing that is allowed according to our schema. A metafield reference for a collection simply can't be an ExternalVideo. I'm somewhat surprised that it allows you to even grab id (although that might be because every type has an id), but the ExternalVideo shouldn't be on the Collection metafield reference at all.
There was a problem hiding this comment.
Awesome, thanks for figuring that out!
|
Compared to #271 , here's a breakdown and some thoughts:
Pros (of this over 271):
storefront.query()typescript types to get automatic type inference workingCons:
graphql()function associates the types automatically with the qraphql queries. Here, we have to do it manually by importing the types from the./index.generatedfileclientpreset (as used in 271) to do this here, maybe, too? Side note: I'm also asking the graphql codegen tools people if theclientpreset could output strings by default as well: GraphQL Code Generator v5 Roadmap dotansimha/graphql-code-generator#8296 (comment)/* GraphQL */. Note that the way we have that magic comment currently actually doesn't work 🤷I go back and forth on keeping the generated files tracked in git or not. Any feelings there?