-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix: React detection and Analytics batch upload #11478
fix: React detection and Analytics batch upload #11478
Conversation
@@ -46,7 +46,7 @@ const trackers = { | |||
}; | |||
|
|||
type TrackerTypes = keyof typeof trackers; | |||
type Trackers = typeof trackers[TrackerTypes]; | |||
type Trackers = (typeof trackers)[TrackerTypes]; |
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.
Had made a change to this file I rolled back, but prettier is really insistent that this line be changed... 🙄
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.
Probably could have opened two tiny PRs since the changes are unrelated but did have one small question (even though I think I know the answer)
{ | ||
credentials, | ||
region, | ||
userAgentValue: getAnalyticsUserAgentString(AnalyticsAction.Record), |
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.
Is this meant to track how often Record
is called on Analytics or how often events are batch sent to Pinpoint?
const elementKeyPrefixedWithReact = k => { | ||
return k.startsWith('_react') || k.startsWith('__react'); | ||
}; | ||
const elementIsReactEnabled = e => { | ||
return Object.keys(e).find(elementKeyPrefixedWithReact); | ||
}; | ||
const allElementsWithId = () => Array.from(document.querySelectorAll('[id]')); |
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.
Nit since this doesn't run often to my understanding but typically we'd want to extract these so they're not recreated every time reactWebDetect()
is run.
Second nit would be to use more meaningful parameter names... it's easy enough to intuit that k
is key or e
is element but it's even easier when it's spelled out for you.
f2b1740
into
aws-amplify:feat/user-agent-enhancements/main
Description of changes
React 18 makes complicated most types of detection. Using a common root element lookup wasn't working consistently. This adds a more robust search mechanism that works on the 3 react 18 apps I've tested.
Also, Analytics batch events where missing the user agent action, which this fixes.
Description of how you validated changes
Tested snippets manually, then by linking changes into a test app.
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.