-
Notifications
You must be signed in to change notification settings - Fork 324
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 Client Analytics Pageview Tracking #29
Conversation
apps/base-docs/docusaurus.config.js
Outdated
@@ -1,3 +1,7 @@ | |||
const dotenv = require('dotenv'); | |||
dotenv.config(); | |||
dotenv.config({ path: '.env.local', override: 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.
Think U probably don't want to do this
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.
Hm, unfortunately this will prove difficult due to the way the Dockerfile build works. We'll only have access to environment secrets at server runtime but not at build time. The whole workaround with NextJS publicRuntimeConfig is to get around this limitation. I don't have a clean solution for Docusaurus off the top of my head sadly.
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 was trying to get around this by using customFields
in the Docusaurus config, like they recommend here. Then in my init code here, I'm using env variable values via the customFields pulled from the generated config file.
I don't have all the context to understand the limitation we run into when deploying to prod, but will this still not work?
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, I think this probably still won't work. There's a problem with the difference between ENV vars available at build time vs ENV vars available at run time. Docusaurus is really oriented around doing everything at build time which makes it tough for us because we inject secret vars at run time.
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.
Do you think my best path forward here is to just set isProd to true, showDebugLogging to false, and paste the API key into the code? That's what was done in this onchainsummer example, but I really wanted to avoid it since dev data will end up getting sent to prod amplitude.
I considered moving the analytics init code out of the client module so I could access runtime env variables with useDocusaurusContext, but then I'd lose access to the client module's onRouteDidUpdate, which I'm using to trigger the pageview events.
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.
Sounds like a good workaround. How would I accomplish the override in this case? Are you suggesting I do something like this for lines in the init code that use env vars: amplitudeApiKey: process.env.amplitudeApiKey || 'prod-api-key'
?
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'm not exactly sure what form the code will take but I think if U experiment with it given the spec 'works by default like prod, overridden in development' I have full confidence U'll figure it out!
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.
My bad, I'd assumed you had something specific in mind. Sounds good though! I'll take a pass at that now.
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.
All good! Thanks for bearing through the difficulties of working in this particular environment :)
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.
Will using env variables via docusaurus customFields cause errors in prod or just be undefined? Assuming they're just undefined, I have a solution that I can commit in the morning. Otherwise, I'll take another pass at this then.
da4f117
to
f0ee23b
Compare
I'm not sure but U can test it by running `yarn workspace @app/base-docs
build` if that doesn't error then the production build should be fine
(refer to the Dockerfile to understand how production works).
…On Mon, 2 Oct 2023 at 19:49, jacob-moore-cb ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In apps/base-docs/docusaurus.config.js
<#29 (comment)>:
> @@ -1,3 +1,7 @@
+const dotenv = require('dotenv');
+dotenv.config();
+dotenv.config({ path: '.env.local', override: true });
Will using process.env.NODE_ENV cause errors in docs prod or just come up
as undefined? If it doesn't cause any errors, I think I can just remove the
dotenv implementation. With that removed, I could go back to referencing
process.env in the docs analytics init code as long as I provide values for
prod to fallback on when they're undefined. So it'd be like this:
- const isDevelopment = process.env.NODE_ENV === 'development'
- isProd: !isDevelopment
- showDebugLogging: isDevelopment
- amplitudeApiKey: isDevelopment ? 'development-key' : 'prod-key'
—
Reply to this email directly, view it on GitHub
<#29 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ42D243ONUDMM323EJNSTX5NHH7AVCNFSM6AAAAAA5LUZ6UGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNJTG4ZTKOBQG4>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
--
Matthew Bunday
http://zencephalon.com
|
Running a build should empirically answer the question
…On Mon, Oct 2, 2023 at 8:07 PM jacob-moore-cb ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In apps/base-docs/docusaurus.config.js
<#29 (comment)>:
> @@ -1,3 +1,7 @@
+const dotenv = require('dotenv');
+dotenv.config();
+dotenv.config({ path: '.env.local', override: true });
Will using env variables via docusaurus customFields cause errors in prod
or just be undefined? Assuming they're just undefined, I have a solution
that I can commit in the morning. Otherwise, I'll take another pass at this
then.
—
Reply to this email directly, view it on GitHub
<#29 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ42D2XOVUJW2RQR6MQB7TX5NJKNAVCNFSM6AAAAAA5LUZ6UGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNJTG44TSMZWHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Yeah, those warnings seem fine, sounds like it'll work |
@@ -1,3 +1,7 @@ | |||
const dotenv = require('dotenv'); | |||
dotenv.config(); |
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 we don't need to call .config
twice, just the second invocation should be enough
* Add Client Analytics pageview tracking to Base Docs app. * Add Client Analytics pageview tracking to Base Web app. * Add import plugin to fix dotenv linting in docusaurus.config.js * Switch to .ts files and disable type-checking and linting. * Update initCCA file extension in docs config. * Add execution environment check before using window variable. * Replace process.env with publicRuntimeConfig. * Remove Amplitude env vars. Remove device id from analytics init files. * Remove duplicate dotenv call.
What changed? Why?
I added pageview tracking to base.org and docs.base.org so the team can gain insight into which pages/docs are visited most. I'm using Client Analytics loaded via a script tag following this onchainsummer example:
Notes to reviewers
Base Web - CCA is loaded with a Next Script in
_app.tsx
and initialized via that component's onLoad prop to prevent issues with async loading.Base Docs - CCA is loaded with an async script tag and initialized in a client module, both configured in
docusaurus.config.js
.How has it been tested?
I ran both apps locally, navigated to many pages of each site, and reviewed the data generated in debug logs and posted to Amplitude.