Shall we add an item to the code review that forbids setTimeout and setInterval?

[samreid]

In phetsims/energy-skate-park-basics#380 (comment) @jonathanolson said:

Please don't use setTimeout, for the reasons it is banned from usage by our code review guidelines.

Should we add entries to the code review checklist forbidding setTimeout and setInterval?

[samreid]

We could catch these even more eagerly if we forbid them with lint rules.

[pixelzoom]

I'd prefer a lint rule to yet-another checklist item.

[zepumph]

I wonder what is best here. I see many setTimeout calls in places that seem fine to me. In aqua, the sonification wrappers, yotta. We use the same lint rules to lint those repos as we do sim code.

Please don't use setTimeout, for the reasons it is banned from usage by our code review guidelines.

I wonder if @jonathanolson can speak to what these "reasons" are. I see tons of usages in joist, as well as implementations in some Timer types in common code. I think it warrants discussion before a blanket lint rule is implemented that isn't helpful because it creates too many false negatives.

[jonathanolson]

I wonder if @jonathanolson can speak to what these "reasons" are. I see tons of usages in joist, as well as implementations in some Timer types in common code. I think it warrants discussion before a blanket lint rule is implemented that isn't helpful because it creates too many false negatives.

The reasons are that if window.setTimeout or window.setInterval are used, then generally it means the sim can't accurately be used to play back event streams in an identical fashion, since it depends on external timing (the setTimeout/setInterval will potentially execute at a different time, particularly if it shouldn't have been executed at all, e.g. if a sim is paused).

In aqua/yotta (and potentially the sonification wrappers), it isn't in the sim, so it wouldn't affect playback.

[zepumph]

Makes sense thanks. I'm in favor of adding this item to the checklist, or somehow branching some eslint code into a Sim only and all lint rules.

[samreid]

If there aren't too many occurrences outside of sim code, those cases could use // eslint-disable directives.

[zepumph]

I think we should explore a lint rule.

[pixelzoom]

There is already an item in the CRC. It was added by @samreid on 12/1/17. The commit message does not indicate that it was added to address this issue.

• All dynamics should be called from Sim.step(dt), do not use window.setTimeout or window.setInterval. This will help support Legends of Learning and PhET-iO.

Adding these to bad-text.js lint rule seems problematic, since we may use those tokens elsewhere.

[pixelzoom]

Having a code-review item seems to have been sufficient for the past 2 years, so I propose that we close this issue and take no further action on the lint rule.

[zepumph]

I agree, but also thought I'd mention that there is bad text for only sims if you want to investigate adding them there.

I'm a proponent of more list rules and less checklist items. This is both for Devs that have been here for ages, but also in helping the onboarding process.

[pixelzoom]

I agree that lint rules are preferred in general. But I do not think that they are always worth the cost in practice, or in this specific case.

[pixelzoom]

10/3/19 dev meeting:

@zepumph is going to check for these in bad-text.js:

window.setTimeout
setTimeout
window.setInterval
setInterval

[zepumph]

See also phetsims/chipper#737 if a regex is easier (I don't think it will be).

[jbphet]

Fixed in Area Builder, checked above. Unassigning myself.

[samreid]

I'm all set, thanks @zepumph. Over to you @jonathanolson

[jonathanolson]

Apologies about the delay! Handled the above cases.

A few have come up, and I'm not confident in handling them myself:

/Users/jon/phet/git/john-travoltage/js/john-travoltage/view/vibrationController.js
  111:1  error  Line contains bad text: 'setTimeout('  bad-sim-text
  114:1  error  Line contains bad text: 'setTimeout('  bad-sim-text

/Users/jon/phet/git/scenery/js/accessibility/speaker/webSpeaker.js
  210:1  error  Line contains bad text: 'setTimeout('  bad-sim-text

/Users/jon/phet/git/tappi/js/view/SelfVoicingLandingDialog.js
  103:1  error  Line contains bad text: 'setTimeout('  bad-sim-text

@jessegreenberg are you familiar with these cases?

[jessegreenberg]

Thanks, these were easy to convert to stepTimer. These were the last ones so I added what was commented out in bad-sim-text in phetsims/chipper@56e2504. @samreid @zepumph is there anything else to do for this issue?

[zepumph]

Very exciting! Thanks