el.querySelectorAll() is not a function (on comment nodes)

Question

el.querySelectorAll() is not a function (on comment nodes)

cb-fred opened this issue 9 years ago · 11 comments

Hi,
Aurelia-i18n 0.5.3 throws an exception in the updateTranslations(el) (i18n.js:136) function if it is called with a comment node as a parameter, as querySelectorAll() is not defined on comment nodes (at least in the latest chrome)

I guess the fix would be simply to ignore comment nodes, by checking (el.nodeType !== 8)

Answer 1 · 2016-07-08T18:01:51.000Z

We have the same issue here. aurelia-i18n version 1.0.0-rc.1.0.0

Answer 2 · 2016-07-08T18:02:40.000Z

@zewa666 Ping.

Answer 3 · 2016-07-13T13:45:11.000Z

I see that the latest wave of update of yesterday for the RC of Aurelia didn't update aurelia-i18n, any news on a fix for this bug and hopefully some more fix to be released soon?

Answer 4 · 2016-07-13T13:47:27.000Z

@zewa666 is on vacation. Everyone needs a rest :) He'll be looking into these issues when he returns, which shouldn't be much longer.

Answer 5 · 2016-07-13T13:47:50.000Z

Pull Requests are welcome as always.

Answer 6 · 2016-07-13T14:36:44.000Z

I just founded that somehow, comments nodes are passed through the function. Since a comment tag in HTML didn't have any property, hence, it doesn't have a querySelectorAll function neither. I made a quick fix to skip those nodes on my local copy but I'm not sure this is the best nor the real solution beside I think it doesn't make sense to parse those comment, except if we can use them to output some translations.

My fix consist of adding this at the beginning of the updateTranslations function
if (el.nodeType === Node.COMMENT_NODE) { return }

To me its a pretty shady looking fix and there should be a better solution. What you think, are comment nodeType should be processed through that function?

Answer 7 · 2016-07-26T07:17:19.000Z

@maroy1986 and @cb-fred thanks for pointing out the issue. Actually more checks are needed for stuff like undefined elements or text-nodes as such I've fixed up the updateTranslations method a bit. Should be available in the upcoming release

Answer 8 · 2016-07-26T11:33:16.000Z

Thanks @zewa666! I have to admit that was easy and more straight forward than I thought to fix after looking to your commit :)

Answer 9 · 2016-07-28T06:13:26.000Z

@maroy1986 and @cb-fred the new release is published, please check out whether it fixes your issue

Answer 10 · 2016-07-28T12:27:46.000Z

I confirm it fix my issue. Everything's good now! Thanks!

Answer 11 · 2016-07-28T12:44:15.000Z

Great, going to close this, thanks for checking back