feat(other): MERC-7797 Added feature to render Widgets in the regions of supported page types. #726

jkanive · 2021-06-29T01:44:25Z

What?

Added logic to rendered the widgets in the respective regions of a page. This is done by making GraphQL request for each page request and setting the content via the paper handlebar helper setContent.

Tickets / Documentation

Add links to any relevant tickets and documentation.

MERC-7797

@junedkazi Would appreciate your input on this PR.

lib/content-api-client.js

server/lib/page-type-util.js

server/plugins/renderer/renderer.module.js

bc-dhuynh · 2021-07-02T20:43:37Z

server/plugins/renderer/renderer.module.js

+        const cachedGraphQLResponse = cache.get(graphQLDataReqSignature);
+
+        if (internals.options.useCache && cachedGraphQLResponse) {
+            ({ regionResponse } = cachedGraphQLResponse);


What does this mean?

It's extracting cached value of regionResponse instead of making a graphQL call.

jkanive · 2021-07-07T16:10:26Z

♻️ Addressed comments.

christensenep · 2021-07-07T17:49:13Z

lib/content-api-client.js

+ * @param {number} entityId
+ * @returns {Promise<renderedRegions: array}>}
+ */
+async function getRenderedRegionsByPageTypeAndEntityId({


Kinda surprised these need to be two separate functions, isn't really just affecting the query slightly when you don't supply an entity id?

Oh wait right as I clicked the button I realized that parsing the response is slightly different too, yeah fair enough.

When using the GraphQL Playground to build the query it pretty handy because it does the validation of whether a pageType needs entityId or not. I am guessing that was the reasoning for having separate function as its difficult to make a argument conditionally required on the same function.

Oh sorry I was speaking of these functions, not the graphql query itself. But anyway yeah given that it's a different graphql field think this makes sense.

christensenep · 2021-07-07T17:50:34Z

server/lib/page-type-util.js

+    UNSUBSCRIBE: 'UNSUBSCRIBE',
+};
+
+const templateFileToPageTypeMap = {


Really wish we could fetch this from somewhere or include it as a library or something.

This mapping is in Storefront also now, so we technically are doing this in 3 places. BCApp, Storefront and now in Stencil CLI 🤷‍♂️

Are we in hurry with this ticket, so we are introducing here tech debt? If so, lets create a ticket to address it by adding an endpoint to Storefront (or other more relevant service)

christensenep · 2021-07-07T17:52:39Z

server/lib/page-type-util.js

+        return false;
+    }
+
+    return pageTypeWithEntityId.indexOf(pageType) >= 0;


Is this actually necessary? Can we not just check if the entityId is present and just trust that it's correct rather than checking another list we have to maintain here?

christensenep · 2021-07-07T17:54:29Z

server/plugins/renderer/responses/pencil-response.js

@@ -137,6 +138,7 @@ class PencilResponse {
        this.data.context.in_production = false;

        paper.addDecorator(makeDecorator(request, this.data.context));
+        paper.setContent(this.data.renderedRegions);


What?!? My mind is blown.

We have paper helper which we use in Storefront renderer as well which takes input of an object with keys as the regionName and value as the html. So I am formatting it and just feeding it here. I was doing literal search a replace before I thought of figuring out how storefront renderer does it.

Oh yeah sorry that was not actually a question, I was just amazed how simple it was. I know you told me before about this, just seeing it is kind of amazing.

christensenep · 2021-07-07T17:57:07Z

server/plugins/renderer/renderer.module.js


 const networkUtils = new NetworkUtils();

 const internals = {
    options: {},
    cacheTTL: 1000 * 15, // 15 seconds
+    graphQLCacheTTL: 1000 * 300, // 5 minutes


Just a thought for the future, it might be worth considering literally never refreshing the cache without user intervention, or making it a cli setting or something. Just imagining in the vast majority of use cases the user wouldn't be expecting or needing the widgets to update. (I don't really have a sense for how many users are using stencil cli at any given time, admittedly it's probably only a handful so not a huge deal at the moment).

Mostly just reacting to this sort of being a weird in-between solution that doesn't serve anyone that well. If you do want to see widgets changing (if you're working on them at the same time as you use stencil cli), 5 minutes is really long and you're probably better off restarting stencil cli each time you change them. And if you don't, there's no need to ever refresh the cache.

There is an option to start the stencil-cli without using cache stencil start --no-cache. So it might be work changing the TTL to say 60mins or more. I think the TTL is not optional and would throw an error if not provided.

jkanive · 2021-07-07T19:31:09Z

♻️ Addressed comments.

jairo-bc · 2021-07-08T08:29:09Z

lib/content-api-client.js

+        });
+
+        const {
+            site: { content },


is content variable for readability?
It's possible to do like this:

Suggested change

site: { content },

const { site: { regions: { renderedRegionsByPageType } } } = response.data.data

jairo-bc · 2021-07-08T08:31:17Z

lib/content-api-client.js

+            renderedRegions: regions,
+        };
+    } catch (err) {
+        console.log(err);


console.error

jairo-bc · 2021-07-08T08:35:59Z

lib/content-api-client.js

+            renderedRegions: regions,
+        };
+    } catch (err) {
+        console.log(err);


console.error
It's also possible to highlight error with red color like here https://github.com/bigcommerce/stencil-cli/blob/master/lib/release/release.js#L145

jairo-bc · 2021-07-08T08:36:49Z

server/index.js

@@ -27,6 +27,8 @@ function buildManifest(srcManifest, options) {
    pluginsByName['./plugins/renderer/renderer.module'].customLayouts =
        options.dotStencilFile.customLayouts;
    pluginsByName['./plugins/renderer/renderer.module'].themePath = options.themePath;
+    pluginsByName['./plugins/renderer/renderer.module'].storeUrl =
+        parsedSecureUrl.protocol + '//' + parsedSecureUrl.host;


Please create a variable storeUrl and reuse it on line 16 as well

jairo-bc · 2021-07-08T08:43:35Z

server/lib/page-type-util.js

+    UNSUBSCRIBE: 'UNSUBSCRIBE',
+};
+
+const templateFileToPageTypeMap = {


Are we in hurry with this ticket, so we are introducing here tech debt? If so, lets create a ticket to address it by adding an endpoint to Storefront (or other more relevant service)

jkanive · 2021-07-12T19:23:39Z

Are we in hurry with this ticket, so we are introducing here tech debt? If so, lets create a ticket to address it by adding an endpoint to Storefront (or other more relevant service)

@jairo-bc I have created a ticket MERC-7975 to address the mapping tech debt and will look into it in a follow up PR.

jkanive · 2021-07-12T19:27:20Z

♻️ Addressed all the comments.
cc: @jairo-bc @christensenep

jairo-bc

LGTM

jkanive force-pushed the MERC-7797-1 branch 2 times, most recently from 2899f97 to 3f483f3 Compare June 29, 2021 02:12

jkanive requested review from bookernath, kchu93, powerwlsl, bc-dhuynh and bc-jcha June 29, 2021 03:17

jkanive force-pushed the MERC-7797-1 branch from 3f483f3 to 8cf768c Compare June 29, 2021 23:53

bc-dhuynh reviewed Jul 1, 2021

View reviewed changes

lib/content-api-client.js Outdated Show resolved Hide resolved