Prevent infinite loops #125
Conversation
|
This LGTM, but given it is a quite critical functionality, I would actually like if @jankowiakmaria and @pbazydlo could have a look too. |
| var _ = require('underscore'); | ||
|
|
||
| var hashBuilder = require('../../utils/hash-builder'); | ||
| var strings = require('../../resources'); | ||
|
|
||
| var wrapLoops = function(code){ | ||
| var CONST_MAX_ITERATIONS = 1e9; // should this be configurable? |
There was a problem hiding this comment.
Best would probably be to have a default (used during dev) and then get a configurable value on the registry level.
During publishing, CLI would ask the registry for packaging config and then use that value.
But at the moment, I don't care, this is enough.
Can you just remove the comment and perhaps move the value here? https://github.com/opentable/oc/blob/master/resources/settings.js
|
@matteofigus I've made those changes. @jankowiakmaria how scary does it look? |
|
@matteofigus @jankowiakmaria tell you what, I'll pull the source of all the existing components and run it though the new packager and see if anything shakes loose |
|
Thanks, that's a good idea |
|
I've run it against a selection of the components in our Preprod registry; Some I couldn't verify because the components don't link to the repo, but so far nothing I've seen worries me. |
|
@matteofigus so I can't see any immediate problems that will be caused by this. What do you think? Merge? |
|
Let me bump the patch for the other PR I just merged, then I would do a merge + minor for this, ok? give me a couple of mins. |
|
Ok, published in |
closes #124
This approach doesn't necessarily cope with nested loops, insofar as the
__ITERvariable will get redeclared in the lower scope.This means that nested loops will run until the bottom loop hits the limit (not necessarily just the outer). We can guard separately against nested loops if we want to.