bigcommerce / paper-handlebars

Paper plugin for rendering via Handlebars.js
BSD 4-Clause "Original" or "Old" License
12 stars 36 forks source link

Loosen AssignVar Limits #305

Open bobbyshaw opened 4 months ago

bobbyshaw commented 4 months ago

Hi,

Our team at Space 48 recently had a production issue where a customer adding a widget to a global region on a page in PageBuilder broke content rendering on other pages. The cause was the use of assignVar in this widget took them over the 50 vars limit on these pages.

The out-of-hours incident was attended to by our team and supported by BigCommerce support team, however both were unable to immediately identify that this was the cause.

Upon review, the use of assignVar in this project seems reasonable. It has been used in numerous places to simplify the template logic by pulling business logic to the top of the template and using variables as flags or the result of if/else check.

Unfortunately, given the impact and lack of traceability when it occurs we have since discouraged developers from using assignVar and refactored our project to reduce the use of it.

Moving forward, we would like to:

  1. Be able to more easily identify when this is the cause of a storefront outage.
  2. Be able to monitor assignVar use per page
  3. Encourage assignVar use if it helps reduce the complexity of the template.

Identifying this issue

I'm afraid I don't have any ideas here so I'm open to suggestions or education if there's already a log somewhere of stencil issues in production! In our instance, the issue wasn't immediately repeatable by running a copy of the theme locally against production.

Monitoring assignVar use

Our understanding is that the current limit is 50 per page/template. Our challenge is that beyond a check for assignVar use across the entire theme, we don't have a reliable way of monitoring assignVar use per template, especially when you factor in widget placements as well.

Is there or could there be a way for us to monitor the amount of vars on a page? For example, a new helper could return the count of variables. We could use something like this in the footer to keep an eye on real-world usage.

Object.keys(globals.storage.variables).length

Encourage assignVar use

I understand that the limit of 50 was put in place as a safety mechanism. While this is the first time that we've come across it, I wonder if there is any appetite to increase this limit substantially so that it can be used without fear while also still serving as protection for memory usage.

Tagging for visibility: @bookernath @AndrewBarber

Appendix

For your reference, here's a list of 46 assignVars that I've identified in our theme around the time of the incident to give you a flavour of what they were being used for. https://gist.github.com/bobbyshaw/88b1041d791cb9de9aa15ff575611344

AndrewBarber commented 4 months ago

Tickets with BC support for this was 07721161 (may help?). I believe there was a subsequent ticket, however, failing to find it on mobile. 😅

Tiggerito commented 4 months ago

To add to this, assignVar can be used in the script manager script, so the count may be pushed up by apps.

I found using assignVar to be the only solution for one of our apps, as it has to generate raw HTML and can't use JavaScript (A Google Merchant Centre limitation). So, I'm stuck with the crude coding ability of Handlebars.

Where possible, I keep using the same variable name ('WsaTemp1') to minimise the impact. For example, to find a value in a loop and store it temporarily so the subsequent code can use it. We do this often when hunting for a value in product options or custom fields.

But in some cases I need app-wide vars, adding to the count.

My app currently uses up to 5 variables. I'm working on a new Google feature (variant structured data) requiring 8 more variables.

Some more powerful way to store data for later use in a scoped way would be great.

Another pain with assignVar is that it crashes if you try to assign an empty string.