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

Add possibility to use piwik.js via browserify #9876

Merged
merged 2 commits into from Mar 7, 2016
Merged

Add possibility to use piwik.js via browserify #9876

merged 2 commits into from Mar 7, 2016

Conversation

alexbeletsky
Copy link
Contributor

We are using Piwik together with browserify and would like to be able to use require('./piwik.js') to get the module. Unfortunately, it's not possible since Piwik is not assigned to a global namespace with transformations like deglobalify.

This change would explicitly assign Piwik to window, that fixes the issue above.

  • Todo after merge: Build minified piwik.js

@tsteur
Copy link
Member

tsteur commented Mar 6, 2016

That's awesome 👍 I noticed our Tracker tests fail because of JSLint see https://travis-ci.org/piwik/piwik/jobs/113611238#L780. Likely we need to add Piwik here: https://github.com/piwik/piwik/blob/2.16.0/js/piwik.js#L959

Also I noticed there are some more references to Piwik eg https://github.com/piwik/piwik/blob/2.16.0/js/piwik.js#L6490-L6493 . Should we update these as well? I hope it won't break anything re exposing Piwik as an AMD module?

Note: After merge we need to build minified piwik.js

@tsteur tsteur changed the title Use piwik as window reference Add possibility to use piwik.js via browserify Mar 6, 2016
@alexbeletsky
Copy link
Contributor Author

@tsteur Thanks 👍 I've added Piwik to config section.. Regarding https://github.com/piwik/piwik/blob/2.16.0/js/piwik.js#L6490-L6493 , my assumption it is fine there, since local Piwik instance is referred.

@tsteur
Copy link
Member

tsteur commented Mar 7, 2016

Sweet, thank you for this! 👍

tsteur added a commit that referenced this pull request Mar 7, 2016
Add possibility to use piwik.js via browserify
@tsteur tsteur merged commit e49cd78 into matomo-org:master Mar 7, 2016
tsteur added a commit that referenced this pull request Mar 7, 2016
@mattab mattab added this to the 2.16.1 milestone Mar 17, 2016
@mattab mattab added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Mar 17, 2016
@mattab
Copy link
Member

mattab commented Mar 17, 2016

Hi @alexbeletsky not sure, but maybe it would be possible to add an automated test to check the notatation require('./piwik.js') works? maybe around these lines in our JS test suite: https://github.com/piwik/piwik/blob/master/tests/javascript/index.php#L548

@alexbeletsky
Copy link
Contributor Author

@mattab it's hard to say to me, since I'm not that aware of that framework you use for testing.

But, I have to add following: if you use browserify and require('./piwik.js') it won't work properly. What I did is the possibility to use browserify with deglobalify transformation, so in code it looks like,

const Piwik = require('./piwik.js', 'Piwik');

Sure, it would be good idea to make Piwik as real UMD module, it requires bit more work. But could be good next logical step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants