-
Notifications
You must be signed in to change notification settings - Fork 326
fix perf tracking and make it optional from developer's end #1096
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
This comment has been minimized.
This comment has been minimized.
examples/template-hydrogen-default/tests/e2e/performance-metrics.test.js
Show resolved
Hide resolved
...ydrogen/src/foundation/Analytics/connectors/PerformanceMetrics/PerformanceMetrics.server.tsx
Show resolved
Hide resolved
...en/src/foundation/Analytics/connectors/PerformanceMetrics/PerformanceMetricsDebug.client.tsx
Show resolved
Hide resolved
| } | ||
| ); | ||
| } | ||
| }); |
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.
Could maybe add an empty dep array [] and also return a function that cleans up the subscribe?
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 can't - it still double executes. It's either this or that useRef trick documented
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's due to StrictMode, correct? If so, it won't double execute in a real build; only in dev mode.
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 want this to double execute, even in dev. Double firing of analytics should never happen no matter where you are.
mcvinci
left a comment
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.
Thanks for these updates, @wizardlyhel! I added a couple suggestions. Open to your thoughts!
Co-authored-by: Michelle Vinci <[email protected]>
Co-authored-by: Michelle Vinci <[email protected]>
...ydrogen/src/foundation/Analytics/connectors/PerformanceMetrics/PerformanceMetrics.client.tsx
Outdated
Show resolved
Hide resolved
jplhomer
left a comment
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 like the optionality. It adds more to the template, but it's a similar pattern to what Remix does for their root entrypoint.
Co-authored-by: Josh Larson <[email protected]>
* v1.x-2022-07: Fix stale product options (#1210) Upgrade body-parser in hydrogen package (#1232) Add new options to Money and useMoney (#1215) fix links (#1229) Update turbo and instructions for developing `dev` (#1225) Heck - deploy all branches to Oxygen add context for initialvariantid (#1217) Build chunks are inside assets folder (#1211) Upgraded to SFAPI 2022-07 (#1214) [ci] release v1.x-2022-07 (#1205) Make this a patch instead of minor add references to video in file_reference block (#1197) Laying the foundation for building components in isolation (#1188) Make metafields optional within the ProductProvider. Fixes #1127 (#1209) Add README to /templates directory (#1163) fix perf tracking and make it optional from developer's end (#1096) docs fixes (#1204)
Description
Performance tracking has been broken in latest Hydrogen release. This PR is to:
ClientAnalyticsPerformanceMetricsAdditional context
Before submitting the PR, please make sure you do the following:
fixes #123)yarn changeset addif this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning