-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
perf(v2): more performant gtag and analytics plugin #2070
Conversation
Deploy preview for docusaurus-2 ready! Built with commit a76613b |
Deploy preview for docusaurus-preview ready! Built with commit a76613b |
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.
Looks good, thanks for the optimization.
return { | ||
headTags: [ | ||
{ | ||
tagName: 'link', |
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.
Why do we still need preconnect to https://www.google-analytics.com?
I see <script type="text/javascript" async="" src="https://www.google-analytics.com/analytics.js"></script>
in the deploy preview's HTML which seems to be added in by the loaded gtag script. Is that why you do preconnect to google-analytics.com here? If so, it would be good to add a comment as it isn't obvious.
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. because gtag automatically add google analytics script for you. So its better to preconnect
Motivation
I merged #2062 (comment) to wrong branch. But anyway I added some more optimization here
Gtag
We use inject head html hooks, so that the script head is not appended after JS load. Removing this code
We should move this
docusaurus/packages/docusaurus-plugin-google-gtag/src/gtag.js
Lines 40 to 51 in 6155115
Doing it via client modules previously will put it in main bundle.
Google analytics
Instead of old https://developers.google.com/analytics/devguides/collection/analyticsjs/#the_google_analytics_tag
We use the async tag https://developers.google.com/analytics/devguides/collection/analyticsjs/#alternative_async_tag since our user is mostly not on IE9 or older browser
Summary
Have you read the Contributing Guidelines on pull requests?
yes
Test Plan
Gtag
Production build
Also spec compliant with
https://developers.google.com/analytics/devguides/collection/gtagjs/
Google analytics
https://developers.google.com/analytics/devguides/collection/analyticsjs/#alternative_async_tag