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

TypeError: Cannot read property 'parentNode' of undefined #48

Open
mandarini opened this issue Nov 24, 2017 · 10 comments
Open

TypeError: Cannot read property 'parentNode' of undefined #48

mandarini opened this issue Nov 24, 2017 · 10 comments

Comments

@mandarini
Copy link

I am creating a ReactJS application and I am using piwik-react-router. Everything is running ok. However, when I run a simple test on my App.js, I get the following error:

TypeError: Cannot read property 'parentNode' of undefined
 
      at node_modules/piwik-react-router/index.js:171:6
      at PiwikTracker (node_modules/piwik-react-router/index.js:173:4)
      at Object.<anonymous> (src/components/App.js:14:46)
      at Object.<anonymous> (src/components/App.test.js:3:12)
          at Generator.next (<anonymous>)
          at new Promise (<anonymous>)
      at handle (node_modules/worker-farm/lib/child/index.js:44:8)
      at process.<anonymous> (node_modules/worker-farm/lib/child/index.js:51:3)
      at emitTwo (events.js:126:13)
      at process.emit (events.js:214:7)
      at emit (internal/child_process.js:772:12)
      at _combinedTickCallback (internal/process/next_tick.js:141:11)
      at process._tickCallback (internal/process/next_tick.js:180:9)

I am using jest to run my tests, and I tried to run the default test found here

import React from 'react';
import ReactDOM from 'react-dom';
import App from './App';

it('renders without crashing', () => {
  const div = document.createElement('div');
  ReactDOM.render(<App />, div);
});

I import piwik in my App.js like this:
import PiwikReactRouter from 'piwik-react-router';

The error in piwik-react-router/index.js is on line 171 when it is trying to access s.parentNode. The s is a script element that it just created and got like this:

var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0];

I have set my jest environment to be jsdom, however, it seems that it still cannot find the script.
Is this a known issue? What can I do about this?

@joernroeder
Copy link
Owner

i had a similar issue while i was writing the tests and fixed it quiet dirty: https://github.com/joernroeder/piwik-react-router/blob/master/test/client.tests.js#L13

i didn't find some time to look into this so please let me know if you find a better solution for this.

@mandarini
Copy link
Author

So, the "problematic" piece of code (that creates the problem in tests) is this bit here:

if (opts.injectScript) {
  var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0]; 
  g.type='text/javascript'; g.defer=true; g.async=true; g.src=u+opts.clientTrackerName;
  s.parentNode.insertBefore(g,s);
}

If I understand correctly, this bit seems to add a script tag containing the client tracker's name source to our body (which is essentially the parent node of the first script in the document.

I refactored this code to look like this:

if (opts.injectScript) {
  var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0]; 
  g.type='text/javascript'; g.defer=true; g.async=true; g.src=u+opts.clientTrackerName;
  d.getElementsByTagName('body')[0].insertBefore(g,s);
}

This code does the same thing. It adds a script tag containing the client tracker's name source in our body, since the parentNode of the first script tag is the body.

This solves the issue, because a body always exists, and everything is defined.

Everything else works the same just fine. If you test this and it is ok with you as well, and you plan to implement this in your library, could I make a pull/merge request?

@joernroeder
Copy link
Owner

@mandarini thanks for looking into this. yes s.parentNode.insertBefore(g,s) seems to be the line which causes the problem. But i see some difficulties in changing is as it does exactly the same as piwiks injection https://developer.piwik.org/guides/tracking-javascript-guide which explicitly looks for a <script> tag in the dom.
An improvement i'd recommend is to optionally auto-inject a <script> tag during tests if none is already present.

@mandarini
Copy link
Author

You are right. However, I think that the piwik injection script (like your script) just wants to make sure that the piwik script is the first one to be added in the DOM (that's why it tries to insert the newly created scirpt tag (g in your code) before the first instance of a scirpt element in the body.
I tried auto-injecting a script tag as you do in your tests, but still I get the same error.
It's ok, I will just use my fork for my project because I see no other solution. :)

@joernroeder
Copy link
Owner

yes. it also decides to append itself to the head or body. what did you tried? I think this is a valid use case and piwik-react-router shouldn't break your tests.

@Viscosity4373
Copy link

Viscosity4373 commented Jun 14, 2018

had the same problem, when testing with jest + jsdom.
I tried adding a script tag to the initial jsdomBody before each test as you did, but that did not resolve the issue.

So I forked the package and applied @mandarini's suggestion:
d.getElementsByTagName('body')[0].insertBefore(g,s);
This seems to work fine.

Maybe you can pull her commit:
mandarini@f8e1181

@Viscosity4373
Copy link

Viscosity4373 commented Jun 15, 2018

Ok, I was to quick in my earlier comment, that change only works if there actually is a script tag on the body, but breaks when all scripts are in the head.

So I just added a check if the regular way works (finding the parent node - either head or body) and simply append the script node if it fails.

Now my test suite works, as well as the packages' test suite.

I left the 'dirty' workaround in the piwik-react-router test suite, because three of the client tests explicitly insert piwik in the regular way (which assumes an existing script tag)

update: I changed the test suite to only add an empty script tag, when the test explicitly requires it.

@vinuganesan
Copy link

@fargerio How did you add an empty script tag, when the test explicitly requires it? Can you provide some examples?
Thanks!

@Viscosity4373
Copy link

@vinuganesan like this, just using vanilla js at the beginning of each test that requires an empty script tag to be present:

      // add script tag for piwik initialization to find
      let emptyScriptTag = document.createElement('script');
      document.getElementsByTagName('body')[0].appendChild(emptyScriptTag);

This is the corresponding commit: 73ea7f6

@vinuganesan
Copy link

@fargerio Thanks. Now working fine

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

No branches or pull requests

4 participants