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

Fix childNodes error #65

Merged
merged 1 commit into from
Apr 1, 2016
Merged

Fix childNodes error #65

merged 1 commit into from
Apr 1, 2016

Conversation

martynchamberlin
Copy link
Contributor

This PR fixes a problem where in certain scenarios (e.g. when in Google Chrome with their Google Analytics Opt-out Add-on extension enabled) a page gets the following JS error:

`TypeError: Cannot read property 'childNodes' of undefined`

Ben Nadel has an excellent video addressing this exact thing. When you modify the DOM in Angular by inserting new JS files, you want to be sure that you do this after Angular is all initialized. Ben recommends forcing things back a tick by surrounding things in a $timeout but binding to $window load feels like a better solution.

I'm creating this PR to fix a real-life problem, and I'm not alone, judging from #44. I've confirmed that this fixes the problem while maintaining functionality, and tests are passing.

@pc035860
Copy link
Owner

pc035860 commented Apr 1, 2016

Hi @martynchamberlin ,

Any reason that you choose to use the window's load event rather than the original async $timeout code presented in Ben Nadel's article?

@martynchamberlin
Copy link
Contributor Author

Hi @pc035860 if you'd strongly prefer $timeout then that's okay. I don't think Ben's decision was deeply founded — he admitted in his video that he was just making a solution that would be a quick fix for them to get the error to go away.

If we were going to go with the approach of forcing a new digest cycle, it might be better to go with $rootScope.$apply instead of $timeout. Feels less hacky.

Thoughts?

@pc035860
Copy link
Owner

pc035860 commented Apr 1, 2016

@martynchamberlin
I think there's no need for an additional $apply. And I notice that in Ben's code, he uses $timeout with the no $apply argument on purpose.

$timeout(fn, 0, false);

As for the script injection timing, I'd prefer $timeout rather than load event.
I'm afraid that under some circumstances, we don't get to catch the load event in time, and the module will fail to initialize.

@martynchamberlin
Copy link
Contributor Author

@pc035860 Using $timeout works on our project just as well so I've updated this PR to use that way. Tell me what you think!

@pc035860
Copy link
Owner

pc035860 commented Apr 1, 2016

Good enough for me as well! Thanks for the PR.

@pc035860 pc035860 merged commit 2cd634a into pc035860:master Apr 1, 2016
@martynchamberlin
Copy link
Contributor Author

👍 Thank you!

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