Closed joernroeder closed 6 years ago
Is this PR going to get merged?
@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?
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)
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,
});
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')
);
But overall I'm happy about how things worked out. Thank you for this package!
My god, I didn't realize until now you created this today! I thought this was some forgotten PR made months ago. Sorry!
@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.
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!
You don’t have to provide anything. You’re just able to use a different attribute name... More tomorrow.
@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')
);
@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 -->
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.
@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
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"]]
@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.
toSource()
is only available on objects in Firefox though. I’m not sure how to extract those values from the window._paq
object.
ok, i'll have a look, it should be documented somewhere in the official piwik documentation.
@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?
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>
@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.
@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.
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 9a87e154350713c0308ba305dc1e7a8645210384 works though.
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
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.
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
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!
@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.
@larsnystrom i've updated the master branch! could please check it out, i'll push it to npm afterwards…
That works!
perfect! i've just published the new version v0.12.1
to npm. thanks again for your work 👊
Thank you! 👏
Coverage remained the same at 100.0% when pulling e7b671cead99d9614bc8c389eaeb66617685419c on already-initialized into eb54dea92c2bfe16b1b7716e8a4ff1daf2106dc6 on master.