support getting bulk elements for tables based on spec#66
Conversation
| collectTableSources(toGet, tableJson.body); | ||
| } | ||
| } catch (e) { | ||
| } |
There was a problem hiding this comment.
You should throw something, no? Otherwise, you're making a silent error with this change that will be difficult to find when it fails (as it must, otherwise we wouldn't be "trying", right?).
There was a problem hiding this comment.
in this case we just want it to continue as a failed json parse isn't necessarily an error and is not critical
There was a problem hiding this comment.
well, I can't argue with that logic. Though, I disagree with it whenever I see it, as it feels like we could stuff a json response of "failed to parse", without specifying actual error handling behavior and at no real cost. . .
I do like abdicating error handling, but I dislike wasting an opportunity to help the "me" that has to debug later. ;)
| } | ||
| toGet = toGet.concat(data); | ||
| }).finally(function() { | ||
| var toGetSet = new Set(toGet); |
There was a problem hiding this comment.
I don't think you should reference toGet as you only assumed success in the 'then' clause. This will mean that, regardless of success or failure of the promise to return, your finally is NOW dependent on success. Based on the content of this finally clause, the code within it should probably be moved up, and the original $http.get call should be chained with the 'getElements' promise being made in the finally clause. Of course, if the code below doesn't change, referencing a variable that may not exist on failure of the original promise is a moot point. Or, I could be totally misunderstanding: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally
There was a problem hiding this comment.
toGet is declared in the higher context, if the get succeeds it just adds to it, otherwise we proceed with what we already had
| var row = body[i]; | ||
| for (j = 0; j < row.length; j++) { | ||
| var cell = row[j]; | ||
| for (k = 0; k < cell.content.length; k++) { |
There was a problem hiding this comment.
something like underscore.js or even jQuery should have an iterable class that would allow for less error prone iteration of the table to non-recursively and with constant memory do what's being done here, right? This looks like a loop-unrolling optimization, honestly.
There was a problem hiding this comment.
the simple for loop is usually fastest compared to other library based iterations, and we deal with a lot of huge tables, unless we need a function scope for each element, we prefer for loops for dealing with tables
borromeotlhs
left a comment
borromeotlhs
left a comment
There was a problem hiding this comment.
If this works, it will be because it just has. However, I believe that there are some simple things that can be done in this file that would set a better precedent for future changes and refactoring with minor investment. The benefit is that if none of the concerns are met, and we can still 'get it to work for now' as we all know none of us receives full time funding to do this as a day job ;)
3 participants
this should also fix portal performance for mounted project element,