joernroeder / piwik-react-router

Piwik analytics component for react-router
https://www.npmjs.org/package/piwik-react-router
MIT License
61 stars 22 forks source link

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

Open mandarini opened 6 years ago

mandarini commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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.

fargerio commented 6 years ago

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: https://github.com/mandarini/piwik-react-router/commit/f8e11812080f884b088ade694afdcbdcf7b7b2d7

fargerio commented 6 years ago

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 commented 3 years ago

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

fargerio commented 3 years ago

@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: 73ea7f60a1852eee8ea99e49ff4c47bf9d3c345f

vinuganesan commented 3 years ago

@fargerio Thanks. Now working fine