esify assumes foo is defined with "for x, y of foo"

[WillsonSmith]

Right now when translating a for of coffeescript loop, we use Object.entries(object) so:

for code, option of @selectedShipmentOptions[@rateCode]
  rows.push(Shopify.Templates.render(@shipmentOptionsTemplate, {
    optionName: option.name,
    optionPrice: @money(option.amount)
  }))

becomes

for (const [code, option] of Object.entries(this.selectedShipmentOptions[this.rateCode])) {
  rows.push(Templates.render(this.shipmentOptionsTemplate, {
    optionName: option.name,
    optionPrice: this.money(option.amount),
  }));
}

This assumes that this.selectedShipmentOptions[this.rateCode] is defined, but that is not always the case. Since this is how it is translated, Object.entries(undefined) throws. I can get around it by doing something like:

const selectedShipmentOption = this.selectedShipmentOptions[this.rateCode]) || {};
for (const [code, option] of Object.entries(selectedShipmentOption) {
...

but ideally this wouldn't be necessary.

@GoodForOneFare

[GoodForOneFare]

160+ instances of this when decaffing app/assets/javascripts/**/*.coffee 😬 At the very least, we should add an esify warning.

[GoodForOneFare]

@lemonmade this seems like an esify blocker. Should we:

• Tweak decaf to add || {} to any Object.keys/Object.entries calls in for for/of loops
• Add an esify warning to check for nulls in for/of loops
• Add a shopify-codemod to figure out loops with potential null references and add fallback values
• [your suggestion here]

[lemonmade]

Yes, I agreed, we need to fix. Are we even converting to something equivalent? Object.entries() is only over own properties, it looks like CS does it over all properties?

We might want to just match the simpler for in loop directly from CS, unless that violates some linting rule I don't remember. If so, I would just stick to || {}.