Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Fix the basic recursion error using setTimeout #1014

Merged
merged 5 commits into from
Feb 2, 2015

Conversation

tweettypography
Copy link
Contributor

Using setTimeout breaks up the call stack. This fixes #774

Update the solution to improve render performance

Use css to set the header size

Fix the repeater's recursion error using setTimeout

Fix the basic recursion error using setTimeout
@kevinparkerson kevinparkerson self-assigned this Jan 29, 2015
@@ -638,7 +645,7 @@
callbacks[stage](null);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grunt is complaining about these unnecessary semicolons. There's another on line 650. Symptom of converting from the variable-declared functions I imagine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Just a typo

@kevinparkerson
Copy link
Contributor

Tested this out, it allows for a HUGE number of rows to be rendered (I tried 10,000) which is pretty awesome. However, speed is significantly slower, even at 100 rows for example. Not sure if there's anything that can be done about that.

@tweettypography
Copy link
Contributor Author

I might be able to improve the speed a little without sacrificing too much. Let me take a look. For an average number of rows I felt like the difference wasn't very noticeable, and for large numbers the grid never worked in the first place, but I'll see if I can tweak slightly

@tweettypography
Copy link
Contributor Author

Updated. 10,000 rows might still be a bit slow but more typical numbers shouldn't suffer the same fate

@@ -603,7 +609,13 @@
if(index<renderer.nested.length){
loopNested(cont, index);
}else{
proceed('complete', args);
if (++skip % 50 === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make this value a configurable option so that this can be optimized best for individual repeater performance. Perhaps a name like `recursionLimit' or something along those lines

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets wait on that to see if this can be done without recursion altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we have an API that is based on callbacks for the render / sub-render steps. I tried a lot of different solutions but kept running into the same exponential growth because of that. Prior to realizing I could use setTimeout as a sort of trick/hack I was feeling like we were going to have to deprecate the API and start over (which might still be worthwhile but is obviously a breaking change)

@futuremint
Copy link
Contributor

Merging this interim solution for now.

futuremint added a commit that referenced this pull request Feb 2, 2015
Fix the basic recursion error using setTimeout
@futuremint futuremint merged commit 41dc1cf into ExactTarget:master Feb 2, 2015
@interactivellama interactivellama added this to the 3.6.0 milestone Feb 9, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repeater - too much recursion
4 participants