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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add codegen to demo-store #937

Merged
merged 26 commits into from
May 26, 2023
Merged

Add codegen to demo-store #937

merged 26 commits into from
May 26, 2023

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented May 24, 2023

Related #887
Closes #939

This PR adds generated types to most of the files of the the demo-store. There are still 2 or 3 remaining that require heavier changes.

I've fixed some bugs while doing this, mostly due to missing fields in queries that were supposed to be used later for SEO or related.

Questions:

  • For changelogs... would this become demo-store@2.0.0?
  • It looks like flattenConnection has a problem with some of the generated types. Some times, the generated type has {edges: {node: Array<Stuff>}} but flattenConnection expects {edges: {node?: Array<Stuff>}}, with that optional ?. @frehner any idea how to fix this? 馃

@frandiox frandiox requested a review from a team May 24, 2023 12:04
Comment on lines +396 to +402
totalTaxV2 {
...Money
}
totalPrice {
totalPriceV2 {
...Money
}
subtotalPrice {
subtotalPriceV2 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were accessing xyzV2 in code but not requesting it here... 馃く

Comment on lines 232 to 236
...CustomerDetails
}
}

fragment CustomerDetails on Customer {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracting this as a fragment so that we can import the CustomerDetailsFragment generated type.

Comment on lines +16 to +18
const policies = Object.values(
data.shop as NonNullableFields<typeof data.shop>,
).filter(Boolean);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This NonNullableFields utility is just representing what the filter(Boolean) is doing, removing null from the array.

@github-actions

This comment has been minimized.

Comment on lines 130 to 138
featuredData: any; // @todo: help please
featuredData: Promise<{
featuredProducts: ProductCardFragment[];
featuredCollections: FeaturedCollectionDetailsFragment[];
}>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@juanpprieto 馃槵

@blittle
Copy link
Contributor

blittle commented May 24, 2023

If there's an edges property, can we always assume there will be an edges.node property?

@frehner
Copy link
Contributor

frehner commented May 24, 2023

If there's an edges property, can we always assume there will be an edges.node property?

Not necessarily. Someone could query

collections() {
  edges {
    cursor
   }
}

hypothetically. https://shopify.dev/docs/api/storefront/2023-01/objects/CollectionEdge 馃し

@frehner
Copy link
Contributor

frehner commented May 24, 2023

It looks like flattenConnection has a problem with some of the generated types. Some times, the generated type has {edges: {node: Array}} but flattenConnection expects {edges: {node?: Array}}, with that optional ?. @frehner any idea how to fix this? 馃

Can you point me to an example of where this issue is? Thanks!

@frandiox
Copy link
Contributor Author

frandiox commented May 25, 2023

Can you point me to an example of where this issue is? Thanks!

Sure, right here:

// @ts-ignore TODO: Fix flattenConnection types

There are a few // TODO like this in the PR but this is the one I was trying to fix and sadly couldn't.

Perhaps related to using (first: $pageBy, after: $cursor) in the query? 馃

@frehner
Copy link
Contributor

frehner commented May 25, 2023

@frandiox thanks! I put up a PR to fix things in flattenConnection() #945

Copy link
Contributor

@blittle blittle left a comment

Choose a reason for hiding this comment

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

LGTM, also pulled the branch and tried to regression test the whole demo-store.

@blittle
Copy link
Contributor

blittle commented May 25, 2023

For changelogs... would this become demo-store@2.0.0?

I'm not sure how to version the demostore. What is a breaking change when anything we add requires the merchant to update their store if they want the applied changes? I'd be fine with leaving it as minor, unless someone else has a strong opinion

@frehner
Copy link
Contributor

frehner commented May 25, 2023

flattenConnection changes have been merged, so you should be able to update from upstream and get those fixed.

Copy link
Contributor Author

@frandiox frandiox May 26, 2023

Choose a reason for hiding this comment

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

@juanpprieto This is what I meant yesterday in our sync. A component can also export a GQL fragment that can be imported in parent queries that use this component. This way the component decides what it needs, and it can use the generated xyzFragment type as props.

Thoughts?

@frandiox frandiox merged commit b219552 into 2023-04 May 26, 2023
10 checks passed
@frandiox frandiox deleted the fd-codegen-demo-store branch May 26, 2023 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

demo-store uses v2 attributes in some areas still
4 participants