Skip to content

Commit

Permalink
Fix issue #18
Browse files Browse the repository at this point in the history 
In Safari, there are some non DOM nodes in the
elements array. That's why there is an issue.
Now we check whether we've parantNode before invoking it
  • Loading branch information
@arunoda
arunoda committed Oct 28, 2015
1 parent 5894281 commit c0ce85d
Showing 1 changed file with 7 additions and 3 deletions.
10 changes: 7 additions & 3 deletions lib/both.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,13 @@ DocHead = {
removeDocHeadAddedTags() {
if (Meteor.isClient) {
const elements = document.querySelectorAll('[dochead="1"]');
for (let key in elements) {
if (elements.hasOwnProperty(key)) {
elements[key].parentNode.removeChild(elements[key]);
// Elements is not an array. It's an iterator and es5-shim
// does not add .forEach method to it
// That's why we need to use a for-in loop
for(let key in elements) {
const element = elements[key];
if(element.parentNode) {
element.parentNode.removeChild(element);
}
}
}
Expand Down

6 comments on commit c0ce85d

@vladshcherbin
Copy link
Contributor

Choose a reason for hiding this comment

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

@arunoda Since we are using ecmascript package, I think we can use for-of as it uses babel and we support for-of functionality.

So, it will be

const elements = document.querySelectorAll('[dochead="1"]');
for (let element of elements) {
  element.parentNode.removeChild(element);
}

@arunoda
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I am good with it. Send me a PR. And keep the if block.

@vladshcherbin
Copy link
Contributor

Choose a reason for hiding this comment

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

@arunoda I've checked the example in the docs, it says, we can use for-of to iterate the NodeList normally. The babel helps us with ie9+. I checked in browsers too.

So, we don't need this parentNode check. Are you okay with that? I'll send a PR.

@arunoda
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you missed the point. We need that if check. In safari, there are some nodes in the elements which are not DOM nodes. That's what the problem.

Before you send the PR, try whether it's working on safari. Don't just run tests. Try using a real app.

@vladshcherbin
Copy link
Contributor

Choose a reason for hiding this comment

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

@arunoda I understand. There is also length and NodeList prototype. Since they are not iterable objects, they won't be processed with for-of.

I just checked in the real app with blaze and FR. Everything works as expected.

@arunoda
Copy link
Contributor Author

@arunoda arunoda commented on c0ce85d Oct 28, 2015 via email

Choose a reason for hiding this comment

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

Please sign in to comment.