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
Introducing React Hooks for AppInsights #1120
Introducing React Hooks for AppInsights #1120
Conversation
Presently this PR will be blocked as the version of TypeScript in use, 2.5.3, does not support Conditional Types (they were added in 2.8). This was uncovered as I had to update the |
reactPlugin: ReactPlugin, | ||
eventName: string, | ||
eventData: T, | ||
skipFirstRun = 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.
@aaronpowell do you mind explaining why this skipFirstRun
flag is needed here? thanks.
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.
@xiao-lix The reason is to mimic more closely the way the non-hook version works. With useEffect
hooks the effect is triggered on each value update including the initial setting of the value, meaning we would fire off too early and potentially have unwanted events tracked. I explain it more in the post I wrote: https://www.aaron-powell.com/posts/2019-11-19-combining-react-hooks-with-appinsights/#ignoring-unwanted-effect-runs
Is there any update on this PR @xiao-lix? I have a need for this functionality in a project I'm working on with a customer. |
…l/ApplicationInsights-JS into aaronpowell-aaronpowell/issue-991
…into aaronpowell-aaronpowell/issue-991
@@ -20,6 +20,7 @@ export default class ReactPlugin extends BaseTelemetryPlugin { | |||
|
|||
private _analyticsPlugin: IAppInsights; | |||
private _extensionConfig: IReactExtensionConfig; | |||
private _logger: IDiagnosticLogger; |
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.
You don't need to do this.
It's already exposed as
this.diagLog()
if (this._analyticsPlugin) { | ||
this._analyticsPlugin.trackEvent(event, customProperties); | ||
} else { | ||
this._logger.throwInternal( |
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.
So this then becomes
this.diagLog().throwInternal(...)
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 seeing any other blocking issues apart from the intro of the _logger.
For references the diaLog() takes care of the this getting called before initialization() as well.
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.
Remove the _logger and use diagLog() instead
…ationInsights-JS into aaronpowell/issue-991
…ationInsights-JS into aaronpowell/issue-991
Targeting issue #991.
This PR introduces some new features to the React (DOM) package, custom hooks that can be used to track an event or metrics on a component (metric tracking is similar to the HOC
withAITracking
).It is based off the blog post I wrote on the subject of Hooks + AppInsights.