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

[1.0] Fixes 1317 Google Analytics plugin; updates attachHistory listener logic #1318

Merged
merged 5 commits into from
Jul 1, 2017

Conversation

camsjams
Copy link
Contributor

Fixes #1317

GA metrics shows whatever the first SSR-served landing page is because page will never change due to an undefined reference.

Also found a linearly growing page view bug where each call to onRouteUpdate is called more than one time. Added the "solution" to outline the problem but wanted to chat about it since I know we can probably see about preventing the handler from being attached more than once.

Also adds:

  • .editorconfig as there wasn't one (helps IDEs stay sane)

…preventing duplicate counting of pageviews
// Don't track while developing.
if (process.env.NODE_ENV === `production`) {
ga(`set`, `page`, location.pathname)
ga(`send`, `pageview`)
var newPath = newRoute.pathname || newRoute.location.pathname
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KyleAMathews this newPath lastPath thing can be solved somewhere else probably, if you can point me in the right direction!

Essentially, as we navigate through a site, the onRouteUpdate function gets called one additional time, like:

Home page - fires once
... user goes to about page - fires twice
... user goes back to homepage - fires three times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, yeah this code is the cause :-(

history.listen((location, action) => {

Try changing that function so that it only attaches once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the code here, you should do:

exports.onRouteUpdate = function({ location }) {

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 30, 2017

Deploy preview ready!

Built with commit ddccb5c

https://deploy-preview-1318--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 30, 2017

Deploy preview ready!

Built with commit ddccb5c

https://deploy-preview-1318--gatsbyjs.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 30, 2017

Deploy preview ready!

Built with commit ddccb5c

https://deploy-preview-1318--using-drupal.netlify.com

@@ -0,0 +1,19 @@
# editorconfig.org
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice!

@@ -1,7 +1,7 @@
exports.onRouteUpdate = function(location) {
exports.onRouteUpdate = function({ location }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added destructuring, I guess I misread the plugin docs regarding the src folder.

@KyleAMathews
Copy link
Contributor

Deploy preview failed.

Built with commit b4e2c8c

https://app.netlify.com/sites/using-glamor/deploys/5956c80a7960b164cee09a62

history.listen((location, action) => {
apiRunner(`onRouteUpdate`, { location, action })
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, also noticed that this same code exists within:

attachToHistory(routeProps.history)

But the production-app.js file is the root cause

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, root.js is for the development version of Gatsby. It should be fixed too but since you're not generally tracking yourself in development it's not as urgent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well I went ahead and adjusted it too!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect :-)

Thanks! Pulling this in now!

@camsjams camsjams changed the title [1.0] Fixes 1317 [1.0] Fixes 1317 Google Analytics plugin; updates attachHistory listener logic Jun 30, 2017
@KyleAMathews KyleAMathews merged commit f3eeb11 into gatsbyjs:1.0 Jul 1, 2017
@jlengstorf
Copy link
Contributor

Hiya @camsjams! 👋

This is definitely late, but on behalf of the entire Gatsby community, I wanted to say thank you for being here.

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (We’ve got t-shirts and hats, plus some socks that are really razzing our berries right now.)
  2. If you’re not already part of it, we just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. You’ll receive an email shortly asking you to confirm. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again! 💪💜

@camsjams
Copy link
Contributor Author

@jlengstorf thanks! I would love to be part of the maintainers team - I use Gatsby on several large production sites and static WordPress blogs.

@camsjams camsjams deleted the update-google-analytics-plugin branch February 19, 2019 19:27
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

Successfully merging this pull request may close these issues.

4 participants