Skip to main content

IFRAME defer fires

Background

POC deferring iframe: !5925

was not pushed, just initial poc

deferring iframe (caused fire with load event happening before code was executed)

addEvent(window, 'load', deferIframes); was called after window.onload event fired

!6395 (diffs) - why no thumbs up from two other engineers? And no QA notes

we added delay (caused fire with var iframe beings hoisted, so that it was one for all plugins):

!6426 (merged)

fix for iframe being scoped (most likely: caused fire because powr_div was missed which was also hoisted)

!6435 (merged)

\ Final fix which addresses all fires: \ https://gitlab.com/powr/powr/-/merge_requests/6442

\ \ Why didn't we catch those fires?

Main reason is that it is hard to reproduce them, during qa everything worked, due to the async nature of code not all users were getting bugs. Another reason is that code is complicated and it is also hard for reviewers to catch potential bugs (overall 4-5 people reviewed those mr’s in total). Mostly popup caused issues because it is really badly written. 3-4 times before popup was causing issues as well, and alway same parts of its codebase bring troubles.

Learnings of what could we improve

When dealing with powr.js

  1. Make sure two QA engineers testing that and three engineers are included in CRR.

Eliminate strict deadlines

  1. Shopify’s deadline was too strict apart from obvious increasing priority and paying more attention to the tasks like that next time we face a strict deadline we should communicate with stakeholders and move the deadline date further.

Rollout powr.js code in phases

  1. Do that using the alias of the app, or use rollout.

Better highlight what could be broken in the specs

  1. Every time powr.js is touched give additional thoughts on apps behaviour on different platforms and pay close attention to the Popup. Documented here: https://docs.google.com/spreadsheets/d/1eEow-f0mUROg13kknnfhXyCIbkC5Bi0SthXFT6N2nlY/edit#gid=1386834576

Better communication with QA and QA notes

We should ping the QA team in advance so that they could provide the responsible tester which will QA and verify on production.

Refactor popup

First of all let’s better understand popup loading logic in JS.


However, there is no easy answer to popup issues, one thing to do is to admit we have bad code in some places and consider refactoring it (but it is a big project), coz maintaining that code is becoming harder and harder.

Refactor powr.js - Should be on our Roadmap

Also we should consider refactoring powr.js, as it is a dangerous piece of code, everytime we touch it there is a high chance of breaking smth.


 \

We probably need to get rid of powr_staging, powr_local and powr_debug, there has to be one source which builds to correct powr.js. Then we need to re-organize pieces of code into separate modules and methods so that whole logic is easier to understand and debug.

When MR with environment variable announced for the first time 

The engineer should provide information about dynamically changing configuration parameters inside the MR. QA should be aware of these configs to cover additional test cases with/without ENV.


When we create ENV for the first time on production we should request QA to verify that this config doesn't break the functionality.