implemented dynamic already-initialized function to be able to use multiple piwik-react-router instances on the same page

[joernroeder]

No description provided.

[coveralls]

Coverage remained the same at 100.0% when pulling e7b671c on already-initialized into eb54dea on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 9a87e15 on already-initialized into eb54dea on master.

[larsnystrom]

Is this PR going to get merged?

[joernroeder]

@larsnystrom yes! I've worked on this today. Initial issue is #41 and pull request is #42.
I'm currently waiting for feedback and would release it on Friday... any suggestions?

[larsnystrom]

I just started using piwik-react-router today, so this is what I though of: (not everything is related to this PR, sorry about that)

• I'm adding the piwik tracking code to the html document server-side, so I wanted to re-use that piwik instance. This PR solved that, but I had to read the code of the PR to understand how. A little bit more documentation would be nice. In my specific case, I had to add the data-piwik-react-router attribute on the piwik <script> tag on the server-side, and configure piwik-react-router like this:

const piwik = PiwikReactRouter({
    enableLinkTracking: false, // Already enabled in tracking code, should perhaps be disabled by default when piwikIsAlreadyInitialized()?
    ignoreInitialVisit: true,
    piwikScriptDataAttribute: 'piwik-react-router',
    trackErrors: true,
    updateDocumentTitle: false,
});

• I'm using React Router 4, but there's not much documentation on how to integrate with v4. When googling how to get hold of the history instance most SO answers say one should use the HOC withRouter, but that's not going to work in this case. Here's what I did:

import React from 'react';
import { render } from 'react-dom';
import { Router } from 'react-router-dom';
import createHistory from 'history/createBrowserHistory';
import PiwikReactRouter from 'piwik-react-router';
import App from 'containers/App';

const piwik = PiwikReactRouter({
    enableLinkTracking: false, // Already enabled in layout.phtml
    ignoreInitialVisit: true,
    piwikScriptDataAttribute: 'piwik-react-router',
    trackErrors: true,
    updateDocumentTitle: false,
});

const history = createHistory();

render(
    <Router history={piwik.connectToHistory(history)}>
        <App />
    </Router>,
    document.getElementById('app')
);

• I wish there was some way to delay pushing to piwik when the route changes, because my app won't change the document title until after the route has changed so the page title is wrong in all of the stats. I'm not sure this is a clean way to do this, but I was thinking it might be possible to configure piwik-react-router with some function which would take the new path and return a promise to wait for before sending the stats.

But overall I'm happy about how things worked out. Thank you for this package!

[larsnystrom]

My god, I didn't realize until now you created this today! I thought this was some forgotten PR made months ago. Sorry!

[joernroeder]

@larsnystrom i'll look into this tomorrow. but yes the feature request is a couple of month old but personally a static flag felt to unflexible to me therefore i'm trying to find a better solution.

[larsnystrom]

The solution I would've wanted when I started would've been to automatically find the global piwik instance without me having to add a data- attribute to the script tag, but this is definitely good enough. Thanks again!

[joernroeder]

You don’t have to provide anything. You’re just able to use a different attribute name...
More tomorrow.

[joernroeder]

@larsnystrom as you are adding the piwik script tag on the server side you don't have to mess around with piwik-react-router and the <script> element itself. If you'd like to use it just set injectScript: false on all piwik instances and they won't touch the script element at all and just depend on it as a the global variable.

Imagine an other case – and this is what this pull request is trying to fix – where you will instantiate at least two React-Apps/Root-Components on a page, both independent from each other (e.g. a chat and a photogallery) and both try to add its own piwik instance to the page. Now just one of them will make it and the other one will use that instance too.

import React from 'react';
import { render } from 'react-dom';
import { Router } from 'react-router-dom';
import createHistory from 'history/createBrowserHistory';
import PiwikReactRouter from 'piwik-react-router';
import App from 'containers/App';

const piwik = PiwikReactRouter({
    enableLinkTracking: false, // as the piwik instance is shared you don't have to disable it here. it's just resetting true to true otherwise.
    ignoreInitialVisit: true,
    trackErrors: true,
    updateDocumentTitle: false,
    injectScript: false
});

const history = createHistory();

render(
    <Router history={piwik.connectToHistory(history)}>
        <App />
    </Router>,
    document.getElementById('app')
);

[joernroeder]

@larsnystrom
instead of "overriding" some of piwiks default configuration you could also remove trackPageView and enableLinkTracking from the Javascript Tracking Client and set the options on piwik-react-router which leads to less configuration parameters as you can go whith the defaults.

const piwik = PiwikReactRouter({
    trackErrors: true,
    updateDocumentTitle: false,
    injectScript: false
});

<!-- Piwik -->
<script type="text/javascript">
  var _paq = _paq || [];
  // remove it here _paq.push(['trackPageView']);
  // remove it here _paq.push(['enableLinkTracking']);
  (function() {
    var u="//{$PIWIK_URL}/";
    _paq.push(['setTrackerUrl', u+'piwik.php']);
    _paq.push(['setSiteId', {$IDSITE}]);
    var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0];
    g.type='text/javascript'; g.async=true; g.defer=true; g.src=u+'piwik.js'; s.parentNode.insertBefore(g,s);
  })();
</script>
<!-- End Piwik Code -->

[larsnystrom]

Yes, I tried injectScript: false, but I got the error Warning: PiwikTracker cannot be initialized! You haven't passed a url and siteId to it.. So I made those variables global in the server-side code, so I could access them on the client, but then I figured I might as well just use this feature branch instead. It felt like duplication to set those things both on the server and the client.

[joernroeder]

@larsnystrom hmmm…… Could you try and remove _paq.push(['setSiteId', {$IDSITE}]); from the original piwik tracking script and see if they are throwing any errors or warnings. I'm thinking about delegating the responsibility of a valid instantiation of piwiks tracker to the "outside world" if injectScript: false and would remove the need for the siteId option in this case.

Could you also add a console.log(window._paq) at line 57 and post the output with and without _paq.push(['setSiteId', {$IDSITE}]); – sorry, i don't have a similar setup to test it myself right now. Maybe we can just check the existence of a previously added siteId there and throw the warning otherwise.
cheers

[larsnystrom]

I think that's a brilliant idea.

If I remove the 'setSiteId' from the server-side code, what happens is the Piwik server returns 400 (Bad request) when the first action is sent, because the idsite parameter is empty.

console.log(window._paq.toSource()) in Firefox with setSiteId on the server:

[["trackPageView"], ["trackVisibleContentImpressions"], ["enableHeartBeatTimer"], ["enableLinkTracking"], ["setTrackerUrl", "http://localhost:3002/piwik.php"], ["setSiteId", "1"], ["setUserId", "lars"]]

Without setSiteId on the server:

[["trackPageView"], ["trackVisibleContentImpressions"], ["enableHeartBeatTimer"], ["enableLinkTracking"], ["setTrackerUrl", "http://localhost:3002/piwik.php"], ["setUserId", "lars"]]

[joernroeder]

@larsnystrom perfect – that is exactly what i was looking for! I'll add some tests with your window._paq.toSource() examples and remove the need for a siteId accordingly.

[larsnystrom]

toSource() is only available on objects in Firefox though. I’m not sure how to extract those values from the window._paq object.

[joernroeder]

ok, i'll have a look, it should be documented somewhere in the official piwik documentation.

[coveralls]

Coverage remained the same at 100.0% when pulling 4d030c3 on already-initialized into eb54dea on master.

[coveralls]

Coverage decreased (-0.8%) to 99.18% when pulling 6f3ba53 on already-initialized into eb54dea on master.

[joernroeder]

@larsnystrom i've updated the library and added additional tests but i had to push the variables to window._paq directly (see 6f3ba53) which differs from the default piwik instatiation. Could you please check how you instantiated them in your test?

[larsnystrom]

I think I'm pretty close to the default piwik installation. _paq is defined as a global, so accessing it from window._paq when it's not in scope sounds pretty reasonable? Here's my setup:

<script type="text/javascript" data-piwik-react-router>
	var _paq = _paq || [];
	/* tracker methods like "setCustomDimension" should be called before "trackPageView" */
	_paq.push(['trackPageView']);
	_paq.push(['trackVisibleContentImpressions']);
	_paq.push(['enableHeartBeatTimer']);
	_paq.push(['enableLinkTracking']);
	(function() {
		var u=<?= json_encode($this->piwikUrl) ?>;
		_paq.push(['setTrackerUrl', u+'piwik.php']);
		_paq.push(['setSiteId', <?= json_encode($this->piwikSiteId) ?>]);
		<?php if (!$this->user->isGuest()) : ?>
		_paq.push(['setUserId', <?= json_encode($this->user->getUsername()) ?>]);
		<?php endif; ?>
		var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0];
		g.type='text/javascript'; g.async=true; g.defer=true; g.src=u+'piwik.js'; s.parentNode.insertBefore(g,s);
	})();
</script>

[joernroeder]

@larsnystrom thanks for looking into this up again. I also looked in piwiks tracker script and found this jslint comment which sets _paq as a global (predefined) variable. It also adds it here to the global space. I will add an additional test to fix coverage and merge it in tomorrow.

[coveralls]

Coverage decreased (-0.8%) to 99.206% when pulling 0f16ef9 on already-initialized into b64b29b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 95aa781 on already-initialized into b64b29b on master.

[larsnystrom]

This should probably be setUserId: setUserId (colon, not comma)

[joernroeder]

oh, how could that pass the tests 🤔

[coveralls]

Coverage remained the same at 100.0% when pulling ea1d39e on already-initialized into b64b29b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 687ed25 on already-initialized into b64b29b on master.

[joernroeder]

@larsnystrom i have to be afk tomorrow therefore i can't publish a new version to npm right now but feel free to use "piwik-react-router": "joernroeder/piwik-react-router" in your dependencies in the meantime.

[larsnystrom]

I can't get 0.12.0 to work as expected. I'm getting the Warning: PiwikTracker cannot be initialized! You haven't passed a url and siteId to it. message and page views aren't getting logged. Commit 9a87e15 works though.

[joernroeder]

hm, i think we run into the global._paq issue again.

I think I'm pretty close to the default piwik installation. _paq is defined as a global, so accessing it from window._paq when it's not in scope sounds pretty reasonable?

var _paq = _paq || []; from piwiks init code binds a) an existing global variable (window._paq) to the local _paq variable or b) – and i think this is your case – creates an array and binds it (just) to the local variable. therefore please try to swap the line with window._paq = window._paq || []; and see if it works.
cheers

[larsnystrom]

No, I'm still getting the same message when I change all refernces to _paq to window._paq. I added some console statements to the piwikIsAlreadyInitialized function to debug. The first thing i noticed is that the getBaseUrl() returns the wrong URL, since I'm hosting piwik on a different host than the app. This means that the script tag is not found.

When I fixed that (by adding the correct url to the config) I found out that window._paq.length is undefined, even though window._paq is defined. The _paq object is not an array, but something else.

[joernroeder]

i've digged a bit more into piwiks current implementation and i think i've found a solution. check out my latest changes --> https://github.com/joernroeder/piwik-react-router/blob/master/index.js#L59

[larsnystrom]

Yes, that seems to do the trick! The last thing I would like to suggest is to replace the check for a script tag with a check if window._paq.push is defined. The thing is that the piwik host url is not available on the client, since it is decided by the server, so the script tag check will fail in my case.

I think a check for

typeof window !== 'undefined' && 
typeof window._paq !== 'undefined' && 
typeof window._paq.push === 'function'

should be all we need. The push method is the only thing piwik-react-router uses on the _paq object, right?

Thank you for all your work!

[joernroeder]

@larsnystrom yes that sounds like a much simpler solution to detect piwiks existence. Testing the existence of window is already done before piwikReactRouter gets initialized.

[joernroeder]

@larsnystrom i've updated the master branch! could please check it out, i'll push it to npm afterwards…

[larsnystrom]

That works!

[joernroeder]

perfect! i've just published the new version v0.12.1 to npm. thanks again for your work 👊

[larsnystrom]

Thank you! 👏