Storefront v2

[janus-reith]

Resolves #661
Impact: breaking
Type: refactor

Issue

TBD
Refactor to Hooks, Context instead of Mobx, i18n, latest nextjs + MUI and much more.

Solution

TBD

Testing

TBD

[mikemurray]

There's some commented out code in various files that I'd like to see addressed. Some of them can probably be removed like the commented out imports from the component-library.

I'm not sure why the tack event stuff is commented out, or if that code will be revisited in the future. I feel like you may address it in a future PR, but I wanted to be sure.

[willopez]

Fantastic work! This makes the storefront much more useful right out of the box. Thank you again for this great effort. Left a couple of comments/questions and some small change requests.

[willopez]

A couple of more issues I fount:

1. Building the production images fails due to the movement of the source files. Please update the Dockerfile accordinly.

2. When building the production build by executing yarn build I see the following errors:

Error occurred prerendering page "/de/product/-". Read more: https://err.sh/next.js/prerender-error:
ReferenceError: ErrorPage is not defined

error-fetching-graphql Error: Tag not found: {"response":{"errors":[{"message":"Tag not found","locations":[{"line":3,"column":5}],"path":["tag"],"extensions":{"code":"NOT_FOUND","exception":{"eventData":{},"error":"not-found","isClientSafe":true,"reason":"Tag not found","details":{},"stacktrace":["ReactionError: Tag not found","    at Object.tag (file:///usr/local/src/app/src/core-services/tags/queries/tag.js:26:11)","    at runMicrotasks (<anonymous>)","    at processTicksAndRejections (internal/process/task_queues.js:94:5)"]}}}],"data":{"tag":null},"status":200},"request":{"query":"\nquery tagQuery($shopId: ID!, $slugOrId: String!) {\n    tag(shopId: $shopId, slugOrId: $slugOrId) {\n      ...TagInfo\n    }\n}\nfragment TagInfo on Tag {\n    _id\n    position\n    name\n    slug\n    isTopLevel\n    subTagIds\n    heroMediaUrl\n    metafields {\n      key\n      namespace\n      scope\n      value\n    }\n    displayTitle\n}  \n","variables":{"shopId":"cmVhY3Rpb24vc2hvcDpOUUxhdnljYmdtUHhBWDh6cw==","slugOrId":"-"}}}
    at GraphQLClient.<anonymous> (/usr/local/src/app/node_modules/graphql-request/dist/src/index.js:116:35)
    at step (/usr/local/src/app/node_modules/graphql-request/dist/src/index.js:40:23)
    at Object.next (/usr/local/src/app/node_modules/graphql-request/dist/src/index.js:21:53)
    at fulfilled (/usr/local/src/app/node_modules/graphql-request/dist/src/index.js:12:58)
    at processTicksAndRejections (internal/process/task_queues.js:94:5) {
  response: { errors: [ [Object] ], data: { tag: null }, status: 200 },

3. In the browser console, I see a the following warnings and one error:

Warning: React.createFactory() is deprecated and will be removed in a future major release

That warning is coming from the component library, we could just create an issue in that repo and resolve it there. Or just don't use the QuantityInput component. This could be done in another PR, so that this one does not keep getting bigger.

Unknown event handler property `onItemClick`. It will be ignored.
    in div (created by CatalogGrid__GridItem)
    in CatalogGrid__GridItem (created by ContainerQuery)

Error above is due to the tracking code, we could refactor this or just remove it.

Overall, great work! Looking forward to merging this.

[willopez]

Also found the following security vulnerabilities:

Issues to fix by upgrading:

  Upgrade next@9.3.0 to next@9.3.2 to fix
  ✗ Prototype Pollution [Medium Severity][https://snyk.io/vuln/SNYK-JS-MINIMIST-559764] in minimist@0.0.8
    introduced by next@9.3.0 > mkdirp@0.5.1 > minimist@0.0.8 and 26 other path(s)
  ✗ Path Traversal (new) [Medium Severity][https://snyk.io/vuln/SNYK-JS-NEXT-561584] in next@9.3.0

Issues with no direct upgrade or patch:
  ✗ Timing Attack [Medium Severity][https://snyk.io/vuln/SNYK-JS-ELLIPTIC-511941] in elliptic@6.5.1
    introduced by next@9.3.0 > webpack@4.42.0 > node-libs-browser@2.2.1 > crypto-browserify@3.12.0 > browserify-sign@4.0.4 > elliptic@6.5.1 and 1 other path(s)
  This issue was fixed in versions: 6.5.2
  ✗ Information Exposure [Low Severity][https://snyk.io/vuln/SNYK-JS-KINDOF-537849] in kind-of@6.0.2
    introduced by next@9.3.0 > fork-ts-checker-webpack-plugin@3.1.1 > micromatch@3.1.10 > kind-of@6.0.2 and 186 other path(s)
  This issue was fixed in versions: 6.0.3

Looks like upgrading to the latest version (9.3.4) of Next.js should fix them.

[janus-reith]

Also found the following security vulnerabilities:

Issues to fix by upgrading:

  Upgrade next@9.3.0 to next@9.3.2 to fix
  ✗ Prototype Pollution [Medium Severity][https://snyk.io/vuln/SNYK-JS-MINIMIST-559764] in minimist@0.0.8
    introduced by next@9.3.0 > mkdirp@0.5.1 > minimist@0.0.8 and 26 other path(s)
  ✗ Path Traversal (new) [Medium Severity][https://snyk.io/vuln/SNYK-JS-NEXT-561584] in next@9.3.0

Issues with no direct upgrade or patch:
  ✗ Timing Attack [Medium Severity][https://snyk.io/vuln/SNYK-JS-ELLIPTIC-511941] in elliptic@6.5.1
    introduced by next@9.3.0 > webpack@4.42.0 > node-libs-browser@2.2.1 > crypto-browserify@3.12.0 > browserify-sign@4.0.4 > elliptic@6.5.1 and 1 other path(s)
  This issue was fixed in versions: 6.5.2
  ✗ Information Exposure [Low Severity][https://snyk.io/vuln/SNYK-JS-KINDOF-537849] in kind-of@6.0.2
    introduced by next@9.3.0 > fork-ts-checker-webpack-plugin@3.1.1 > micromatch@3.1.10 > kind-of@6.0.2 and 186 other path(s)
  This issue was fixed in versions: 6.0.3

Looks like upgrading to the latest version (9.3.4) of Next.js should fix them.

Yes, will update nextjs, altough Im still waiting for a stable v9.3.5 release and was unsure wether we could just use a canary version - v9.3.5-canary.8 fixed some cpu/memory leaking issue and increases performance for fallback creation a lot.

[janus-reith]

A couple of more issues I fount:

1. Building the production images fails due to the movement of the source files. Please update the Dockerfile accordinly.
2. When building the production build by executing yarn build I see the following errors:

Error occurred prerendering page "/de/product/-". Read more: https://err.sh/next.js/prerender-error:
ReferenceError: ErrorPage is not defined

error-fetching-graphql Error: Tag not found: {"response":{"errors":[{"message":"Tag not found","locations":[{"line":3,"column":5}],"path":["tag"],"extensions":{"code":"NOT_FOUND","exception":{"eventData":{},"error":"not-found","isClientSafe":true,"reason":"Tag not found","details":{},"stacktrace":["ReactionError: Tag not found","    at Object.tag (file:///usr/local/src/app/src/core-services/tags/queries/tag.js:26:11)","    at runMicrotasks (<anonymous>)","    at processTicksAndRejections (internal/process/task_queues.js:94:5)"]}}}],"data":{"tag":null},"status":200},"request":{"query":"\nquery tagQuery($shopId: ID!, $slugOrId: String!) {\n    tag(shopId: $shopId, slugOrId: $slugOrId) {\n      ...TagInfo\n    }\n}\nfragment TagInfo on Tag {\n    _id\n    position\n    name\n    slug\n    isTopLevel\n    subTagIds\n    heroMediaUrl\n    metafields {\n      key\n      namespace\n      scope\n      value\n    }\n    displayTitle\n}  \n","variables":{"shopId":"cmVhY3Rpb24vc2hvcDpOUUxhdnljYmdtUHhBWDh6cw==","slugOrId":"-"}}}
    at GraphQLClient.<anonymous> (/usr/local/src/app/node_modules/graphql-request/dist/src/index.js:116:35)
    at step (/usr/local/src/app/node_modules/graphql-request/dist/src/index.js:40:23)
    at Object.next (/usr/local/src/app/node_modules/graphql-request/dist/src/index.js:21:53)
    at fulfilled (/usr/local/src/app/node_modules/graphql-request/dist/src/index.js:12:58)
    at processTicksAndRejections (internal/process/task_queues.js:94:5) {
  response: { errors: [ [Object] ], data: { tag: null }, status: 200 },

1. In the browser console, I see a the following warnings and one error:

Warning: React.createFactory() is deprecated and will be removed in a future major release

That warning is coming from the component library, we could just create an issue in that repo and resolve it there. Or just don't use the QuantityInput component. This could be done in another PR, so that this one does not keep getting bigger.

Unknown event handler property `onItemClick`. It will be ignored.
    in div (created by CatalogGrid__GridItem)
    in CatalogGrid__GridItem (created by ContainerQuery)

Error above is due to the tracking code, we could refactor this or just remove it.

Overall, great work! Looking forward to merging this.

That looks like some little oversights, will try to fix both tomorrow.
Regarding 3, yes I noticed that aswell and didn't take further action as it originates from the component library and is just a warning. But as it really looks weird and might disturb developer ux, I'm happy to quickly rewrite it in the storefront with just MUI.

[willopez]

We can update to the current latest version of next now, and when the new one come out, it can be updated and it should be a quick new PR.

[janus-reith]

We can update to the current latest version of next now, and when the new one come out, it can be updated and it should be a quick new PR.

Upgraded to 9.3.5 in janus-reith@c06f841

[janus-reith]

Some things are a bit hard to keep track, I made a small list of things left TBD:

• Recheck setSearch and remove or replace it in context/RoutingContext.js
• Migrate components/ProfileContainer and containers/catalog/withCatalogItems to hooks and remove the Provider in lib/apollo/withApollo to be able to remove @apollo/react-components and apollo-client
• Check and fix the Dockerfile for production builds, make sure it works for default setups when the repo has no override for dev mode.
• Remove or replace custom/analytics/provider.example.js and custom/analytics/segment.js depending on how we implement this.
• Resolve the React.createFactory() warning by moving that component off the libary and into a new simple MUI component.
• Avoid styling issues resulting from library version mismatches by removing all references to the component library and move things over to the storefront repo.
• Find out what causes error-fetching-graphql Error: Tag not found. This occurs during the fetch for static data called in getStaticProps. We could consider adding a log to console for each of the functions I created in staticUtils, to better determine what slug/_id/name was used in a call, also later durng runtime if a page is revalidated or created during fallback.

Soon but not immediately:

• Add new Docs, primarily describing the patters how the hooks and Providers for React Context and static data fetching works.

Maybe later:

• "-" slug in "/de/product/-" which caused ErrorPage is not defined. I could simply resolve the error by removing the ErrorPage and using a simple Typography component, at some point I want to move this to MUI skeleton loaders. But the reason that the error page would actually show is the way fallback works: I need to provide a list of slugs to be used for getStaticPaths which must not be empty, the rest would be fallback-created during runtime. Therefore I added "-" as the slug for the product page created during build as we don't know the product pages. We could instead do something like fetching the first 10 products during build, but that would not work for cases where we build a storefront and have no products, and we would also need to use a stub in that case, so as there is no "perfect" solution for now we can just leave it this way.
• Expanding upon custom/analytics: We could still keep this pattern, as it is not directly related to react-tracking and could also work for other scripts. The general pattern used in there to add scripts to _document.js could use another discussion though, as for this to be actually helpful IMHO it needs to be more sophisticated, like allowing both inline or linked scripts or async/defer defined per script.

[janus-reith]

DCO failed again after one of my commits was a revert, and I have no idea why.
I tried to use --amend as DCO suggest, and also rebased everything after it still failed, and Github shows me that 0893b04 is verified properly - DCO still fails stating that the sign-off is missing.

[willopez]

looking at commit 0893b04 I don't see the Signed-off-by: signature, so DOC is correctly reporting that is not signed off. The instructions DOC provides on how to resolve unsigned commits only apply if the unsigned commit was the last one pushed, so it will not work for older commits :( unfortunately. I believe you can go back in git history and squash(merged) the unsigned commit with one that is signed and that should do it. Here is a guide: https://www.atlassian.com/git/tutorials/rewriting-history

[janus-reith]

@willopez Thanks, will try again.
If I click that commit, Github shows it as verified in a green box.
As signing just the last commit as described by DCO expectedly did not work, I already tried rebasing everything twice, starting from the first commit I made in this branch.
Looking at the commit history here, everything seems verified:
https://github.com/janus-reith/example-storefront/commits/storefront-v2
But you're right, that "Signed-off-by" is missing from that Revert commit.

I might rebase again and this time remove both the original commit and its revert in the process, still I'd like to figure out what's going on.

[janus-reith]

I just fixed the build of the docker container container for production builds, here are some things to note:

• The API needs to be accessible during build

• I created .env.prod which links to the default setup and excluded that in .gitingore from being ignored. You can pass a build arument NEXTJS_DOTENV, which will then be used after the dot, e,g, NEXTJS_DOTENV=abc would result in a file .env.abc being picked up. prod is the default fallback value, but I'm open to alternative approaches. I mainly used this myself to be able to generate different deployments from the same source code.

• During build, the graphl utility I made in staticUtils/graphQLRequest.js checks for the IS_BUILDING_NEXTJS env var, and if true defaults to fetching from the external api URL as it is assumed that the build container won't share a docker network with the api container

• Currently, when running on the same host, you need to pass --network=host so the build container is able to locate the ap running at localhost:3000, so the call might be: docker build --network=host -t reactioncommerce/example-storefront:3.0.0 .
  There might be other ways to resolve the route to the API from inside the container, but I found none yet. Also, that concern might be irrelevant when running in a CI and the API is running on some external host anyway.

I expect some adjustment to be necesary so your CI will build the generic image with that, but for now the configuration will work out to build the image locally.
One issue that might come up: During build on a CI, process.env.EXTERNAL_GRAPHQL_URL will be used to fetch data for the static pages, and from then will be inlined to be used by the withApollo HOC clientSide, in other words, you wont be able to just spin up the image and pass your own value for that. This is a security measure by nextjs to avoid leaking sensitive env vars to the client.
process.env.INTERNAL_GRAPHQL_URL on the other hand will only be used internally when static fallback pages are created during runtime.
I'm currently considering to introduce a third variable, BUILD_GRAPHQL_URL, to allow for a scenario like when the default non-dev example image is built on a CI:
BUILD_GRAPHQL_URL could be something external like api.reactioncommerce.com, while EXTERNAL_GRAPHQL_URL might still point to localhost and INTERNAL_GRAPHQL_URL to the internal docker network.

In general, some of the things regarding env vars tend to be tricky, as nextjs inlines some vars if defined in next.config.js while some others stay dynamic and developers will need to take a close look at config.js and next.config.js when customizing to see how things behave in client and server contexts.

[janus-reith]

I just included BUILD_GRAPHQL_URL, as there are no real alternatives for decoupled builds that should run locally and it also might be easier this way to comprehend what is used where.

I didn't bother about that until now, as usually I would either use the dev image locally, or have the production build running against the api endpoint it would also be used with later, and I don't expect many workflows to actually make use of deviating BUILD_GRAPHQL_URL/EXTERNAL_GRAPHQL_URL vars except for the demo image, still it is reasonable to be present.

Maybe @focusaurus could chime in on what to do best with the fragmentation of .env files we have now? There now is .env.example which only has the purpouse of being copied over to .env which will be used for dynamic parts like the api route, and third there is .env.prod which currently is expected to be picked up during build. But ideally I would want to deal with this in a separate follow up PR, considering that his might also introduce changes to the platform dev script.

[willopez]

@janus-reith The production builds are working now, this is a good start. If issues arise, we can iterate on the process. Thanks!

[willopez]

LGTM. There are known issues/improvements that will be resolved in subsequent PRs, I'll create tickets for those issues/improvements.

Changes were made

[willopez]

@janus-reith Thanks again for this contribution, I have created issues for the known issues that can be resolved in subsequent PRs.