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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(demo): update shopify driver usage in demo #3457

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from

Conversation

leozhang14
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

Implementation of the Shopify product driver.

[ ] Bugfix
[y] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Old code that does not work. Uses shop to query (not supported in Shopify Storefront 2025/01).

Fixes: N/A

What is the new behavior?

get (url), getAll, getByURL, and getBestSellers should all function properly.

Does this PR introduce a breaking change?

[y] Yes
[ ] No

Has effects on the demo app, as well as the product library since the shopify driver service files have been modified.

Other information

IMPORTANT:

I don't know how to test the get and getByURL methods as they return observables. I will have to look into this more. I am also running into an issue with the testing for getAll and get, although the daffodil demo seems to work?

Chrome 131.0.0.0 (Mac OS 10.15.7) Driver | Shopify | Product | ProductService getAll | getting a list of products should should return an observable array of 20 products by default FAILED
Error: Expected 0 to equal 20.

Chrome 131.0.0.0 (Mac OS 10.15.7) Driver | Shopify | Product | ProductService get | getting a single product should return an observable single product FAILED
Error: Expected undefined to equal 'bd0de172-3fbd-4e17-8e61-f6d73ff876c3'

@leozhang14 leozhang14 requested review from a team as code owners January 21, 2025 03:00
@damienwebdev damienwebdev marked this pull request as draft January 21, 2025 03:35
@damienwebdev damienwebdev changed the title Shopify driver demo (DO NOT MERGE YET) feat(demo): re-add shopify driver to demo Jan 21, 2025
@damienwebdev damienwebdev changed the title feat(demo): re-add shopify driver to demo feat(demo): update shopify driver usage in demo Jan 21, 2025
: reqInfo.utils.createResponse$(() => ({
status: STATUS.NOT_FOUND,
statusText: `Backend ${reqInfo.collectionName} not found or does not support the ${method} request method`,
}));
: undefined;
Copy link
Member

Choose a reason for hiding this comment

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

What is the motivation for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I accidentally deleted this line when I was experimenting with some things in an attempt to debug an issue I was having a while ago. Sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, I didn't mean to make this change and will undo it.

Copy link
Member

Choose a reason for hiding this comment

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

@griest024 @leozhang14 this change is necessary in order to use the Shopify driver with the in-memory backend enabled.

Without it, the passThru feature of the in-memory backend doesn't work as expected.

Copy link
Member

Choose a reason for hiding this comment

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

hmm can you repro the use case in an integration test in a separate PR? I don't follow.

libs/product/driver/shopify/src/product.service.ts Outdated Show resolved Hide resolved
libs/product/driver/shopify/src/product.service.ts Outdated Show resolved Hide resolved
libs/product/driver/shopify/src/product.service.ts Outdated Show resolved Hide resolved
libs/product/driver/shopify/src/shopifyDriverConfig.ts Outdated Show resolved Hide resolved
libs/product/driver/src/createApolloConfig.ts Outdated Show resolved Hide resolved
@griest024
Copy link
Member

I don't know how to test the get and getByURL methods as they return observables. I will have to look into this more.

Run the tests in a browser and check your console for errors. I suspect its because the flushed data doesn't conform to your new ProductNode interface. The ideal solution is to create a testing package (see @daffodil/product/magento/testing) that holds factories that will autogenerate testing data with the appropriate structure.

@griest024
Copy link
Member

@leozhang14 has any code in this PR been generated by AI tools?

@leozhang14
Copy link
Contributor Author

I don't know how to test the get and getByURL methods as they return observables. I will have to look into this more.

Run the tests in a browser and check your console for errors. I suspect its because the flushed data doesn't conform to your new ProductNode interface. The ideal solution is to create a testing package (see @daffodil/product/magento/testing) that holds factories that will autogenerate testing data with the appropriate structure.

This was the issue, thanks. I'll look into the "testing package" for magento more. Just to confirm, it would be more appropriate to make this change in a separate PR, correct?

@leozhang14
Copy link
Contributor Author

@leozhang14 has any code in this PR been generated by AI tools?

Just these lines below were generated with AI, to perform the check in getByUrl to see if any product in the list of products had a matching URL.

const matchedProduct = products.find((edge) => edge.node.onlineStoreUrl === url)?.node;
if (!matchedProduct) {
throw new Error(Product with URL ${url} not found.);
}

Apologies if this was an issue—I wasn’t sure if using AI tools was against guidelines. Could you clarify if this is acceptable? Sorry in advance if I misunderstood.

@griest024
Copy link
Member

@leozhang14 has any code in this PR been generated by AI tools?

Just these lines below were generated with AI, to perform the check in getByUrl to see if any product in the list of products had a matching URL.

const matchedProduct = products.find((edge) => edge.node.onlineStoreUrl === url)?.node; if (!matchedProduct) { throw new Error(Product with URL ${url} not found.); }

Apologies if this was an issue—I wasn’t sure if using AI tools was against guidelines. Could you clarify if this is acceptable? Sorry in advance if I misunderstood.

its allowed but our contribution rules state that you must openly disclose it in the description of your PR. It's not a big deal and we really don't make that notice very prominent but its good for reviewers to know so we can pay extra close attention.

Also, in my personal opinion, while I think AI can be a powerful and potentially valuable tool in programming, its important to make sure you always understand the code that you end up using (whether generated by AI or copied from SO, etc.). AI should be a labor-saving tool not a skill crutch. Overall, whatever results in the best final product is fine by me.

@griest024
Copy link
Member

I don't know how to test the get and getByURL methods as they return observables. I will have to look into this more.

Run the tests in a browser and check your console for errors. I suspect its because the flushed data doesn't conform to your new ProductNode interface. The ideal solution is to create a testing package (see @daffodil/product/magento/testing) that holds factories that will autogenerate testing data with the appropriate structure.

This was the issue, thanks. I'll look into the "testing package" for magento more. Just to confirm, it would be more appropriate to make this change in a separate PR, correct?

Generally, the factories for a model should be introduced in the same PR that the models themselves are. I would be okay with adding them to this PR but if you wanted to extract the model + factory change to its own PR, that's fine too.

@leozhang14
Copy link
Contributor Author

I don't know how to test the get and getByURL methods as they return observables. I will have to look into this more.

Run the tests in a browser and check your console for errors. I suspect its because the flushed data doesn't conform to your new ProductNode interface. The ideal solution is to create a testing package (see @daffodil/product/magento/testing) that holds factories that will autogenerate testing data with the appropriate structure.

This was the issue, thanks. I'll look into the "testing package" for magento more. Just to confirm, it would be more appropriate to make this change in a separate PR, correct?

Generally, the factories for a model should be introduced in the same PR that the models themselves are. I would be okay with adding them to this PR but if you wanted to extract the model + factory change to its own PR, that's fine too.

I will try to add the model + factory changes to this PR as well. Thanks for the advice.

Copy link
Member

@griest024 griest024 left a comment

Choose a reason for hiding this comment

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

I'm liking the changes so far!

Comment on lines 1 to 4
export interface ShopifyMoneyV2 {
amount: number;
currencyCode: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

This model (and the associated factory) should be in @daffodil/driver/shopify as it is a generic model and doesn't have anything to do with products specifically. I really like how you are breaking up the types!

libs/product/driver/shopify/src/models/shopify-product.ts Outdated Show resolved Hide resolved
libs/product/driver/src/createApolloConfig.ts Outdated Show resolved Hide resolved
@damienwebdev
Copy link
Member

@griest024 @leozhang14 instead of manually defining the types, as we have, what if instead we use graphql-codegen to only gen the types our queries use. This does mean that factories may break if we modify the outgoing query, but it will make the maintenance devx a bit nicer.

@damienwebdev
Copy link
Member

damienwebdev commented Jan 24, 2025

Re: #3457 (comment) (IDK why Github wont let me comment in line) I'd vote for @daffodil/driver/graphql I don't intend to support multiple graphql clients simultaneously.

@griest024
Copy link
Member

@griest024 @leozhang14 instead of manually defining the types, as we have, what if instead we use graphql-codegen to only gen the types our queries use. This does mean that factories may break if we modify the outgoing query, but it will make the maintenance devx a bit nicer.

I like this idea. Anything to relieve maintenance burden

@leozhang14
Copy link
Contributor Author

leozhang14 commented Jan 25, 2025

@griest024 @leozhang14 instead of manually defining the types, as we have, what if instead we use graphql-codegen to only gen the types our queries use. This does mean that factories may break if we modify the outgoing query, but it will make the maintenance devx a bit nicer.

Will we implement this using the Shopify CLI? Also, should this change be in this PR, or another one?

@damienwebdev
Copy link
Member

Id say do what works, I have no strong opinion

@leozhang14 leozhang14 marked this pull request as ready for review January 25, 2025 04:55
Copy link
Member

@griest024 griest024 left a comment

Choose a reason for hiding this comment

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

You need to use child factories where appropriate. Factories should only mock out the fields that their corresponding type declares. So ShopifyProductGraph only has one field: nodes, MockShopifyProductGraph should only mock out nodes at the top level and rely on the child factory mock to handle its internal fields.

Comment on lines 9 to 13
{
id: `gid://shopify/ProductImage/${faker.datatype.number({ min: 10000000000 })}`,
url: faker.image.imageUrl(),
altText: faker.random.words(5),
},
Copy link
Member

Choose a reason for hiding this comment

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

Same here, use the child factory

Comment on lines 24 to 30
nodes: [
{
id: `gid://shopify/ProductImage/${faker.datatype.number({ min: 10000000000 })}`,
url: faker.image.imageUrl(),
altText: faker.random.words(5),
},
],
Copy link
Member

Choose a reason for hiding this comment

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

Use ShopifyProductImagesFactory.createMany to generate these. See DaffCategoryFactory for an example of how to use child factories.

Copy link
Member

@griest024 griest024 left a comment

Choose a reason for hiding this comment

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

I like this change!

@@ -0,0 +1,6 @@
{
"$schema": "../../node_modules/ng-packagr/ng-entrypoint.schema.json",
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs three ../, make sure this path resolves.

Copy link
Member

Choose a reason for hiding this comment

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

Remove this empty file

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.

3 participants