-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
chore(rebuild): re-incorporate google-analytics #2045
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
Conversation
src/index.jsx
Outdated
|
||
// Client Side Rendering | ||
if ( window.document !== undefined ) { | ||
ReactDOM.render(( | ||
<BrowserRouter> | ||
<BrowserRouter id={gaID}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component triggers an error locally because id
is required. I think it is fine, haven't tried passing an empty string, or any string for that matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe default gaID
to an empty string and see what happens? If not, we should change it to something like:
import { BrowserRouter, Route } from 'react-router-dom'
import { BrowserRouter as AnalyticsRouter } from 'react-g-analytics'
if ( process.env.NODE_ENV === 'production' ) {
BrowserRouter = AnalyticsRouter
}
Then in dev, the normal BrowserRouter
will simply ignore the id
prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be cool, but import
s are defined as const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
const BrowserRouter = process.env.NODE_ENV === 'production' ? require('react-g-analytics') : require('react-router-dom').BrowserRouter;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes or if we want imports just define another variable:
const Router = process.env.NODE_ENV === 'production' ? AnalyticsRouter : BrowserRouter;
...
<Router ...>
P.S. but i guess require is totally fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like inline require
s. Your first idea is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk I feel like we should stick with ES6 import
s in as much of the codebase as possible for consistency. I forgot about the const
issue but I like the idea of just defining a new var as @EugeneHlushko showed above.
webpack.common.js
Outdated
behaviour: 'append' | ||
} | ||
], | ||
require('remark-html'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you separate these changes to another PR? They seem unrelated to the google-analytics fix.
webpack.dev.js
Outdated
compress: true, | ||
historyApiFallback: true | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I'm not exactly sure what even changed. Was this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier, sorry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you have a sublime plugin for it or something, I see now. Yeah if you don't mind, let's wait to make all those changes as a separate commit until after this is done and merged. Then we can integrate it into the build process in a way that hits everyone's builds.
@montogeek aside from my few comments and figuring out the prod/dev discrepancy, I think this is good to go assuming it works ok. How did you test btw? Seems like a tricky thing to play around with (e.g. you don't want to report stuff from a dev instance). |
I would suggest implementing ga for prod before merging
Otherwise looks pretty straightforward |
@skipjack I built the site and navigate 2 pages, I saw 2 successful requests to Google Analytics server. |
Feedback addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/index.jsx
Outdated
import Site from './components/Site/Site'; | ||
|
||
// TODO: Re-integrate <GoogleAnalytics analyticsId="UA-46921629-2" /> | ||
// Consider `react-g-analytics` package | ||
const Router = process.env.NODE_ENV === 'production' ? AnalyticsRouter : BrowserRouter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use require
here inside a conditional and only import the browser required for produciton. DOn't know if with this approach BrowserRouter
gets dead code eliminated, if it does I'm fine with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it get DCE, will confirm later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing, shouldn't we keep the analyticsId in a dot file so we don't expose it here, though we already did either way I guess 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GA keys are public any way or do you mean just for configuration purposes? We can put it into package.json file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah its safe to expose 👍 moving it out to single source of truth from the template is another question, which i suspect shouldn't be tackled in this PR
No description provided.