feat(data): Add live GraphQL data to product detail page #90

zetlen · 2018-06-26T15:01:54Z

Basic implementation of a GraphQL query for product details. Builds on #52 by replicating the inline query declaration. Had to plumb out the child components for the new data shape; in doing so, I made a few reusable functions.

Closes #87.

This PR is a:

[ ] New feature
[x] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

Added GraphQL query to
packages/venia-concept/src/RootComponents/Product.js.
- Resolves from URL by using the url_key in a GraphQL query.
- ~~Can also resolve by SKU.~~
- Modified prop types and render method to accommodate live data shape.
~~Added <Currency /> component whose signature matches the Magento GraphQl Money type.~~
- ~~Uses the window.Intl standard object to format.~~
Modified the Gallery and ProductImageCarousel components to use new data shape.
Moved shared constant data URIs to a single src/shared folder, to replicate placeholder logic.
~~Created a shared propShapes.js file containing commonly used prop type expressions.~~
Anticipating that url_key would be a common way to navigate, I made a url_key utility function.
Added a makeProductMediaPath utility function, for turning product image file paths from API responses into relative URLs.
- Though magento/graphql-ce/issues/88 is still a problem for production, I found that when magento-sample-data is installed, it symlinks into the pub/media folder so you can use simpler URLs.
- You can see this for yourself with ls -l <magento-root>/pub/media/catalog/product.
- So I added a makePathPrepender function, which we'll later use often, that can create functions like makeProductMediaPath.
- I hardcoded /media/catalog/products in the code, but I also added an environment variable to .env and webpack.config.js for configuring that URL per instance.

Additional information

Todos

Optimize queries with fragments
Centralize queries in query file to be preprocessed
Make link to product detail on category page
Resolve media URL issue
Test with image galleries

DrewML · 2018-06-27T09:24:14Z

packages/venia-concept/src/util/getUrlKey.js

+export default function getUrlKey(url = window.location) {
+    // The URL key is the last path segment.
+    const basename = url.pathname.slice(url.pathname.lastIndexOf('/') + 1);
+    // TODO: this may be configurable, but Magento SEO urls appear to always


this may be configurable

Dave sitting over my shoulder: "Yep, it is"

Is it commonly changed though? And is the current setting available through an API somewhere?

I couldn't answer either of those questions, but it turns out we don't need this code. See #90 (comment)

DrewML · 2018-06-27T09:27:59Z

packages/venia-concept/src/components/Currency/index.js

+import { Component, createElement } from 'react';
+import PropTypes from 'prop-types';
+
+export default class Currency extends Component {


Were you aware that the Price component exists? Not clear if you just missed it, if this replaces it, or if this is in addition to it?

I should have looked at the Price component, but I didn't. It looks like the Price component is a better implementation of this; I just think it should be called Currency, since not every representation of a currency is a price (e.g. subtotals, difference amounts, etc). I'll switch to this component and separately advocate for its renaming.

DrewML · 2018-06-27T09:39:04Z

packages/venia-concept/src/components/Currency/index.js

+        tagName: 'span'
+    };
+
+    guessLocalLanguage() {


We don't need to guess here. The spec advises implementers to use the environment's default. We should trust the browser to do the right thing, unless we have evidence they don't.

Default is accomplished by passing in undefined for the first positional argument to Intl.NumberFormat. See implementation here

Good, that's good news--though I'm removing this implementation anyway.

DrewML · 2018-06-27T09:43:09Z

packages/venia-concept/src/shared/propShapes.js

@@ -0,0 +1,19 @@
+import { shape, number, string, bool } from 'prop-types';


I'm very reluctant to add a "shared proptypes" file. We did this at my previous employer while using GraphQL. We ended up with dozens of permutations of similar-but-slightly-different shapes for each component. We also found it encouraged over-fetching of data since you didn't have to make a 1-off version of a shared proptype if you just added a field or 2 to your query.

The other downside was that you no longer can just look at the file and see the shape of data. You now need to jump through the abstraction to the shared prop files, and hop back and forth between the component and the proptype implementation. This becomes much more painful in PRs when you're reviewing the code in Github. IMHO, there is massive value in being able to see the exact shape of all the data while hacking on the component.

We have to find a compromise on introducing high-level abstraction. They're not always necessary, no, but it's necessary to create some. We have to lower cognitive load for developers, and busy, deep inline definitions are cognitive load.

I see how shared propType shapes could become annoying if you define shared shapes too early, but we will have a shared prop shapes file. It's the equivalent of GraphQL fragments. (In fact, we might later put them together in a convenient wrapper, but not yet.) We can debate what belongs there, but we will need higher-level objects. This exports actual immutable prop types, not plain objects for PropTypes.shape(). If a dev needs a different shape, they have to make it inline. Whereas if they need the same shape, they use the shared shape.

It may make things a little harder for maintainers and code reviewers in exchange, but in cases where that's a tradeoff, our project's mission is to lean in the third party dev's direction. We should keep this file.

I have to agree with @DrewML. This is a path to the dark side. If the product is ever ripe for this optimization, that'll be clear in the future and it'll make a good PR then.

As you said, if a developer needs a different shape, they have to make it inline. That's going to encourage them to use the wrong shape for convenience. I believe we want people to write granular, specific, custom queries on a per-component basis, not use the most convenient existing query.

Thanks to both of you for helping me think about this. I still believe that any time something should be a fragment, it should also be a shared prop type. However, we don't have fragments yet, so it makes sense to remove the shared prop types file. I will!

DrewML · 2018-06-27T09:54:39Z

packages/venia-concept/src/components/ProductImageCarousel/thumbnailList.js

    };

    render() {
-        return <List renderItem={Thumbnail} {...this.props} />;
+        // linear-time sort is possible when we have a numeric 'position' prop


Few notes:

The comment in the top of render doesn't seem necessary - let's remove it.

We may want to reconsider doing this in render. Could be memoized, but we can always address this later

It does not seem ideal to make the client-side do this sorting. Are we sure this list isn't already sorted when returned from the API? If not, we should log a feature request for it to be ordered.

I just checked on my local and that list already comes back from the API sorted on position

I did this because of the presence of a position member in the first place. In my experience, something like that means that collection order is not significant. I'm gonna ask about this real quick.

Makes sense to me. I don't see a problem with this. If items have a position prop, that ought to be the sort order regardless of how the API returns them.

Though I agree it should at least be memoized a bit (i.e., cache only the last result of filter + reduce).

I implemented this as a shouldComponentUpdate! Good for me.

I also checked with the GQL team, and they confirmed that the returned order of the collections is not guaranteed to be position order, and position should win. They should change it if they can, and we should map GQL query results somewhere in our data layer to ensure that array order IS significant, but that's a future optimization.

Lastly, the linear-time sort comment seems like a nice reassurance to me; it's much faster than the O(nlogn) of Array.prototype.sort().

DrewML · 2018-06-27T10:02:40Z

packages/venia-concept/src/util/getUrlKey.js

+ * @param {URL} url
+ * @returns {string} A string for use as the `url_key` in a GraphQL query.
+ */
+export default function getUrlKey(url = window.location) {


Don't think we need any of this - we can already get the product's ID for the product page query. The urlResolver query has a canonical_url field - this will always be in the format catalog/product/view/id/{integer}.

Been meaning to do this anyway. Opened a PR to feed this to the RootComponent #93

Thanks for that! I was halfway through writing that just now.

However, we still need the url_key, because of magento/graphql-ce#86. We could also open an issue asking for the urlResolver to be an interface and we could add inline fragments for things like sku, but that would take longer than just doing it this way.

DrewML · 2018-06-27T10:14:26Z

packages/venia-concept/src/RootComponents/Product/Product.js

+const productDetailQuery = gql`
+query productDetail($sku: String, $urlKey: String) {
+  productDetail: products(filter: { sku: {eq: $sku}, or: {url_key: {eq: $urlKey }}}, pageSize: 1, currentPage: 1) {
+    total_count


Seems like multiple fields in this query aren't used in this PR afaict. Can we audit and remove unused fields?

jimbo · 2018-06-27T17:40:10Z

packages/venia-concept/src/components/Gallery/item.js

@@ -112,6 +113,9 @@ class GalleryItem extends Component {
    /**
     * Product images are currently broken and pending a fix from the `graphql-ce` project
     * https://github.com/magento/graphql-ce/issues/88
+     *
+     * When using sample data, which symlinks to bypass cache,
+     * you can simple prepend /media/catalog/product/, which we'll do from .env.


Good to know. 👍

Will this leave broken images for those not using the sample data? @antonkril said there is a convention we can use to get any product photo with the URL we get back from GraphQL. It will be the full size (for now), but we wouldn't have to hack it

Yeah it would. And that would be awesome--we can hit it next sprint.

jimbo · 2018-06-27T17:43:53Z

packages/venia-concept/src/shared/images.js

    'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABkAAAAfCAQAAAC4ua71AAAAGklEQVR42mNkIBkwjmoZ1TKqZVTLqJYRpgUAaP0AIAQAObYAAAAASUVORK5CYII=';
+
+export const grayPlaceholder = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAQAAAAFCAQAAADIpIVQAAAADklEQVR42mNkgAJGIhgAALQABsHyMOcAAAAASUVORK5CYII=';


None of the original placeholders were gray; they were all transparent. Have you changed one of them to gray or is this misnamed?

Looks like I made a bad assumption based on some CSS, and the fact that those two base64 strings are slightly different. But examining them...they're both transparent and they only differ by size.

Whoops! I'll just use the one transparentPlaceholder.

jimbo · 2018-06-27T17:53:05Z

packages/venia-concept/src/components/ProductImageCarousel/thumbnailList.js

    };

    render() {
-        return <List renderItem={Thumbnail} {...this.props} />;
+        // linear-time sort is possible when we have a numeric 'position' prop


Makes sense to me. I don't see a problem with this. If items have a position prop, that ought to be the sort order regardless of how the API returns them.

Though I agree it should at least be memoized a bit (i.e., cache only the last result of filter + reduce).

jimbo · 2018-06-27T18:03:52Z

packages/venia-concept/src/shared/propShapes.js

@@ -0,0 +1,19 @@
+import { shape, number, string, bool } from 'prop-types';


I have to agree with @DrewML. This is a path to the dark side. If the product is ever ripe for this optimization, that'll be clear in the future and it'll make a good PR then.

As you said, if a developer needs a different shape, they have to make it inline. That's going to encourage them to use the wrong shape for convenience. I believe we want people to write granular, specific, custom queries on a per-component basis, not use the most convenient existing query.

zetlen · 2018-06-27T23:06:11Z

@DrewML @jimbo Thanks for all your input so far. It's especially important for this to merge soon, so I have to ask for you to do followup reviews somewhat quickly. I realize that travel and schedule makes that hard, but I still have to ask.

coveralls · 2018-06-27T23:09:42Z

Pull Request Test Coverage Report for Build 163

1 of 45 (2.22%) changed or added relevant lines in 7 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+2.8%) to 64.695%

Changes Missing Coverage	Changed/Added Lines	%
packages/venia-concept/src/components/ProductImageCarousel/thumbnail.js	2	0.0%
packages/venia-concept/src/util/getUrlKey.js	2	0.0%
packages/venia-concept/src/components/Currency/index.js	7	0.0%
packages/venia-concept/src/util/makeMediaPath.js	8	0.0%
packages/venia-concept/src/RootComponents/Product/Product.js	12	0.0%
packages/venia-concept/src/components/ProductImageCarousel/carousel.js	13	0.0%

Files with Coverage Reduction	New Missed Lines	%
packages/venia-concept/src/components/ProductImageCarousel/carousel.js	1	0.0%

Totals
Change from base Build 145:	2.8%
Covered Lines:	783
Relevant Lines:	1260

💛 - Coveralls

coveralls · 2018-06-27T23:09:42Z

Pull Request Test Coverage Report for Build 219

1 of 39 (2.56%) changed or added relevant lines in 6 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-1.9%) to 61.552%

Changes Missing Coverage	Changed/Added Lines	%
packages/venia-concept/src/util/getUrlKey.js	2	0.0%
packages/venia-concept/src/components/ProductImageCarousel/thumbnail.js	4	0.0%
packages/venia-concept/src/util/makeMediaPath.js	8	0.0%
packages/venia-concept/src/components/ProductImageCarousel/carousel.js	12	0.0%
packages/venia-concept/src/RootComponents/Product/Product.js	12	0.0%

Files with Coverage Reduction	New Missed Lines	%
packages/peregrine/src/ContainerChild/ContainerChild.js	1	0.0%
packages/venia-concept/src/components/ProductImageCarousel/carousel.js	2	0.0%

Totals
Change from base Build 216:	-1.9%
Covered Lines:	796
Relevant Lines:	1361

💛 - Coveralls

DrewML · 2018-06-28T14:02:00Z

@zetlen I can review again in the next few hours once I'm in Valencia

DrewML · 2018-06-28T17:23:19Z

packages/venia-concept/src/RootComponents/Product/Product.js

-                </article>
+                <Query
+                    query={productDetailQuery}
+                    variables={{ urlKey: getUrlKey() }}


Can we add a TODO that links to an issue on the graphql-ce repo with a request to add a filter by entity ID?

I put it above the query definition.

DrewML · 2018-06-28T17:30:47Z

packages/venia-concept/src/components/ProductImageCarousel/carousel.js

    };

+    shouldComponentUpdate({ images }) {
+        return (


I don't think this should be a shouldComponentUpdate. This actually will run (in most cases) just as frequently as render. So now, if nothing has changed, we're looping over all the images 3 times (the filter in render, the sort in render, and the some in here).

The recommended approach for possibly-expensive derivations from props in render is to use a memoization helper. The better alternative is PureComponent, but we're writing these components for third parties, so we can't guarantee they'll always pass in an array with a new identity.

/cc @jimbo to correct me if I'm wrong about anything here

You're correct. There was too much looping before, and this is worse.

As I said in my comment, basic memoization is the solution here. We don't need to cache every result of filter + reduce, only the most recent one, so that render can avoid looping in the case where images have not changed.

I disagree with the premise that we can't assume third parties will avoid mutating array props, but that's a larger subject. I think we should remove this shouldComponentUpdate and memoize instead.

DrewML · 2018-06-28T17:32:51Z

packages/venia-concept/src/components/ProductImageCarousel/carousel.js

    render() {
        const { classes, images } = this.props;
+        // the order of the array is not guaranteed to be the position order,


Can we link to the issue you mentioned creating to ensure the API returns a sorted list?

DrewML · 2018-06-28T17:37:24Z

packages/venia-concept/webpack.config.js

@@ -104,6 +104,9 @@ module.exports = async function(env) {
                    phase === 'production' || enableServiceWorkerDebugging
                        ? serviceWorkerFileName
                        : false
+                ),
+                'process.env.MAGENTO_BACKEND_PRODUCT_MEDIA_PATH': JSON.stringify(


This needs some form of documentation

DrewML · 2018-06-28T17:38:25Z

packages/venia-concept/src/RootComponents/Product/Product.js

+
+                        return (
+                            <article className={classes.root}>
+                                <section className={classes.title}>


Most of the keys we're doing lookups on with classes are missing in the proptypes definition for this component.

CSS classes are too fast-changing to solidify as a type; they're just a dictionary of strings, so I changed the prop type to that.

I think you should discuss this with @jimbo before deviating. There is a good reason these are documented the way they are. They're a public API.

they're just a dictionary of strings

The same argument could be made about prop-types in general: "they're just a dict of values."

There is a good reason these are documented the way they are. They're a public API.

This. If you're writing CSS, you're targeting classnames. If those classnames are changing, your CSS needs changing. That's an interface.

CSS classes are too fast-changing to solidify as a type

This really shouldn't be the case with CSS modules. Since the solution to a problem is never "add a classname to it", the classnames shouldn't really change unless a structure change necessitates it. As with any API, if you're changing it too frequently, you should probably be thinking about it more.

Those are fair points; the objectOf prop type seems a bit useless as a consequence. Let's add all the classnames in a later PR; the missing classes prop types predate this PR, and we don't want this one to get too large. I'll revert the change I made and get rid of the objectOf proptype.

It's ok, I fixed it for you. 🙂

jimbo · 2018-06-28T22:23:01Z

packages/venia-concept/src/components/ProductImageCarousel/carousel.js

    };

+    shouldComponentUpdate({ images }) {
+        return (


You're correct. There was too much looping before, and this is worse.

As I said in my comment, basic memoization is the solution here. We don't need to cache every result of filter + reduce, only the most recent one, so that render can avoid looping in the case where images have not changed.

I disagree with the premise that we can't assume third parties will avoid mutating array props, but that's a larger subject. I think we should remove this shouldComponentUpdate and memoize instead.

zetlen · 2018-07-05T16:26:20Z

Made your requested changes, @DrewML and @jimbo :)

Basic implementation of a GraphQL query for product details. Builds on #52 by replicating the inline query declaration. Had to plumb out the child components for the new data shape; in doing so, I made a few reusable functions. - Added GraphQL query to `packages/venia-concept/src/RootComponents/Product.js`. - Resolves from URL by using the `url_key` in a GraphQL query. - Can also resolve by SKU. - Modified prop types and render method to accommodate live data shape. - Added `<Currency />` component whose signature matches the Magento GraphQl `Money` type. - Uses the `window.Intl` standard object to format. - Modified the `Gallery` and `ProductImageCarousel` components to use new data shape. - Moved shared constant data URIs to a single `src/shared` folder, to replicate placeholder logic. - Created a shared `propShapes.js` file containing commonly used prop type expressions. - Anticipating that `url_key` would be a common way to navigate, I made a `url_key` utility function. - Added a `makeProductMediaPath` utility function, for turning product image file paths from API responses into relative URLs. - Though [magento/graphql-ce/issues/88](magento/graphql-ce#88) is still a problem for production, I found that **when `magento-sample-data` is installed, it symlinks into the `pub/media` folder so you can use simpler URLs.** - You can see this for yourself with `ls -l <magento-root>/pub/media/catalog/product`. - So I added a `makePathPrepender` function, which we'll later use often, that can create functions like `makeProductMediaPath`. - I hardcoded `/media/catalog/products` in the code, but I also added an environment variable to `.env` and `webpack.config.js` for configuring that URL per instance. - Optimize queries with fragments - Centralize queries in query file to be preprocessed - Make link to product detail on category page - Resolve media URL issue - Test with image galleries Closes #87.

zetlen requested review from jimbo and DrewML June 26, 2018 15:02

DrewML reviewed Jun 27, 2018

View reviewed changes

jimbo reviewed Jun 27, 2018

View reviewed changes

zetlen force-pushed the zetlen/live-product-data branch from 70e18c0 to 0d00930 Compare June 27, 2018 23:02

DrewML reviewed Jun 28, 2018

View reviewed changes

jimbo suggested changes Jun 28, 2018

View reviewed changes

DrewML added the pkg:venia-concept label Jun 29, 2018

zetlen force-pushed the zetlen/live-product-data branch 2 times, most recently from 1170dee to 9479533 Compare July 1, 2018 20:23

zetlen force-pushed the zetlen/live-product-data branch from 9fa0bb7 to c49439b Compare July 6, 2018 17:04

jimbo approved these changes Jul 6, 2018

View reviewed changes

zetlen force-pushed the zetlen/live-product-data branch from c49439b to c0bbd1c Compare July 9, 2018 21:26

zetlen merged commit 77b6bd6 into master Jul 9, 2018

DrewML mentioned this pull request Jul 10, 2018

feat(checkout): Integrate live product, cart, checkout #140

Merged

zetlen deleted the zetlen/live-product-data branch September 21, 2018 15:27

		@@ -0,0 +1,19 @@
		import { shape, number, string, bool } from 'prop-types';

		'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABkAAAAfCAQAAAC4ua71AAAAGklEQVR42mNkIBkwjmoZ1TKqZVTLqJYRpgUAaP0AIAQAObYAAAAASUVORK5CYII=';

		export const grayPlaceholder = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAQAAAAFCAQAAADIpIVQAAAADklEQVR42mNkgAJGIhgAALQABsHyMOcAAAAASUVORK5CYII=';

feat(data): Add live GraphQL data to product detail page #90

feat(data): Add live GraphQL data to product detail page #90

Conversation

zetlen commented Jun 26, 2018 • edited Loading

This PR is a:

Summary

Additional information

Todos

DrewML Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrewML Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrewML Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetlen commented Jun 27, 2018

coveralls commented Jun 27, 2018

Pull Request Test Coverage Report for Build 163

💛 - Coveralls

coveralls commented Jun 27, 2018 • edited Loading

Pull Request Test Coverage Report for Build 219

💛 - Coveralls

DrewML commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrewML Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrewML Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetlen commented Jul 5, 2018

zetlen commented Jun 26, 2018 •

edited

Loading

DrewML Jun 27, 2018 •

edited

Loading

DrewML Jun 27, 2018 •

edited

Loading

DrewML Jun 28, 2018 •

edited

Loading

coveralls commented Jun 27, 2018 •

edited

Loading

DrewML Jun 28, 2018 •

edited

Loading

DrewML Jul 2, 2018 •

edited

Loading