Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix to getTemplate(), $http instead of $.ajax() #347

Closed
wants to merge 2 commits into from
Closed

Bugfix to getTemplate(), $http instead of $.ajax() #347

wants to merge 2 commits into from

Conversation

c0bra
Copy link
Contributor

@c0bra c0bra commented Apr 16, 2013

For some reason if an external template (row/cell or header I would guess) is fetched with $.ajax(), and it receives a 304 (not modified) response, a "$digest is already running" error will occur while it is trying to add it to $templateCache.

Moving over to $http to do the get request seems to have fixed it. Was there a reason for the jquery/jquery-ish method?

I'd like to add a test for this, if possible, but it has been randomly breaking my app as 304s occasionally happen so I thought I would submit the pull request for now.

For some, reason if a template is fetched with $.ajax() and it receives a
304, a "$digest is already running" error will occur while it is trying to
add it with $templateCache.

Moving over to $http to do the get request seems to have fixed it.
@orneryd
Copy link
Contributor

orneryd commented Apr 16, 2013

the only problem with doing it async, is that the grid will continue to process and set it self up without the templates in some cases (laggy connections and such). I couldn't find a clean way to have $http do a synchronous call.

@c0bra
Copy link
Contributor Author

c0bra commented Apr 17, 2013

OK, I see. Well:

  1. I was tired and misstated part of my diagnosis. It's not that the error happens just when a 304 occurs, but rather when there is a round-trip ajax call happening within Angular (such as requesting from a $resource) and the template fetch completes before the other request. With a 304, it completes very fast and thus intercepts the other request.

    The error then occurs here: https://github.com/angular/angular.js/blob/master/src/ng/http.js#L928 because, and I'm no angular expert, I think a $digest is already occurring and done() is not checking $$phase before calling $apply(). This might then be a problem with Angular itself? Maybe?

    If not, I wonder if there's a way to work around that but still keep the code synchronous. I will look into that today.

  2. How much effort would be involved in making the template fetching asynchronous, but making sure it happens before rendering and whatever "setup" process relies on them being there?

@c0bra
Copy link
Contributor Author

c0bra commented Apr 17, 2013

This plunker will eventually demonstrate it, in FF, if you refresh it enough: http://run.plnkr.co/plunks/KCpCbE/

I can't get it to reproduce in Chrome, so maybe it could be the hack they put in for FF 20?

@c0bra
Copy link
Contributor Author

c0bra commented Apr 17, 2013

Also, if there are two grids using the same rowTemplate you'll end up fetching the same URI twice.

Additionally, is there a reason for pre-pending the gridId to the cache key for the template? Wouldn't the URI suffice?

@c0bra
Copy link
Contributor Author

c0bra commented Apr 19, 2013

OK, I have a new fix, and a "test" demonstrating the error. I actually found it impossible to write a proper angular test (either via unit, midway, or e2e) that would demonstrate an exception being thrown because of intersecting XHRs.

Anyway, here is the relevant branch: c0bra/ng-grid/fix-external-templates.

There's a new test in workbench/templating that will demonstrate the error. You have to install express (now in the devDependencies), and run scripts/server.js. Then navigate to http://localhost:8000/workbench/templating/external.html. If you watch the console it should show an $apply error. This is because the express server is forcing the template to come back AFTER the data from the resource.

The fix I implemented was to make getTemplate() return a promise, and then create an initTemplates() function that returns the set of all the getTemplate() promises as a single promise. init() now waits for initTemplates() to complete, and the execution code in ng-grid.js calls and waits for grid.init() (grid.js does not call its own init function). So now, any external templates will be loaded (or error out) before the rest of the grid gets instantiated.

If you run grunt debug in that branch and retry the new workbench test, it should run properly.

I can send a pull request if this all looks good to you.

@c0bra c0bra mentioned this pull request Apr 23, 2013
@ghost ghost assigned c0bra Apr 26, 2013
@c0bra
Copy link
Contributor Author

c0bra commented Apr 29, 2013

Closed with 2.0.5 release.

@c0bra c0bra closed this Apr 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants