Skip to content
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

ITelemetryContext.user is sometimes null - setAuthenticatedUserContext throws #1541

Closed
josundt opened this issue Apr 22, 2021 · 7 comments
Closed
Milestone

Comments

@josundt
Copy link

josundt commented Apr 22, 2021

-Package: @microsoft/applicationinsights-web
-Package version: 2.6.1
-Installed using npm, built with webpack
-Using TypeScript

Just upgraded @microsoft/applicationinsights-web from version 2.5.8 to version 2.6.1.

I needed to update our code when I found that ITelemetryContext.user (IUserContext) has added two new methods; setAuthenticatedUserContext and clearAuthenticatedUserContext.

Up till now we have just set ai.context.user to an object literal that implements IUserContext when a user logs in (and set it to null when the user logs out) which worked fine.

Since the IUserContext interface now also requires these two new methods, I changed the code so that instead of setting/clearing the ai.context.user property, I rather call these two new methods ai.context.user.setAuthenticatedUserContext and .clearAuthenticatedUserContext when users log in/out.

The problem I faced is that ai.context.user sometimes is null, and then an exception is thrown.

I believe that ai.context.user is supposed to never be null even if the user has not yet logged in, because how are you then supposed to set the authenticated user context?

It looks to me as if the ai.context.user is initialized asynchronously a bit too late.
It would expect this to initialized to something other than null immediately after ai.loadAppInsights() has been called.

@MSNev
Copy link
Collaborator

MSNev commented Apr 22, 2021

You should NOT EVER be replacing or assigning the ai.context.user object!!!!
You CAN however, directly assign the properties of the existing object.

The User class has actually ALWAYS implemented (and required) these functions Here is the version from 2018, it's just that now the types are correctly declaring them to properly support TS upgrades and previously the setAuthenticatedUserContext and clearAuthenticatedUserContext where class level (prototype) functions, but for minification advantages these are now instance level (during initialization).

Looking at the code the user object is created at the point of construction and only if the runtime environment has a window.

The correct way to set the authenticated user details is via the ai.setAuthenticatedUserContext() (which internally (currently) just delegates to the User version), so if there is no window this would also have (and cause) a similar exception.

Summary

I needed to update our code when I found that ITelemetryContext.user (IUserContext) has added two new methods; setAuthenticatedUserContext and clearAuthenticatedUserContext.

Up till now we have just set ai.context.user to an object literal that implements IUserContext when a user logs in (and set it to null when the user logs out) which worked fine.

Don't!

The problem I faced is that ai.context.user sometimes is null, and then an exception is thrown.

It should not ever be AFTER successful initialization, if initialization fails it may not have been set.

I believe that ai.context.user is supposed to never be null even if the user has not yet logged in, because how are you then supposed to set the authenticated user context?

Correct, and it wont be after initialization which creates it here, the only exception is the above for window-less environments.

Several bugs have also been fixed so the correct context is now exposed (when initialized via the snippet).

It looks to me as if the ai.context.user is initialized asynchronously a bit too late.

The Initialization is not (generally) asynchronously, this may depend on your environment and how the NPM packages chunk the code and require() usage. The actual SDK (by default) will perform the initialization synchronously.

It would expect this to initialized to something other than null immediately after ai.loadAppInsights() has been called.

Yes, it should be...
There are some known issues where the wrong exception / error is displayed during failure (an internal work item exists for this (ie. doesn't currently have a GitHb issue) because there are multiple levels where exceptions are caught / logged and either swallowed or another error is "assumed".... Debugging in the browser Dev tools and setting breakpoints in the catch clauses are currently the only way to correct catch these. You could try the Debug Plugin but as an initialization issue this too may also not initialize...

@josundt
Copy link
Author

josundt commented Apr 23, 2021

@MSNev I'm sorry, I found the problem and the issue was actually on my side.

To my excuse - until recently, your TypeScript typings did not expose any other way to set the user context to my knowledge.

IUserContext.setAuthenticatedUserContext or IUserContext.clearAuthenticatedUserContext was missing in the typings.
You also mentioned that there is a method ai.setAuthenticatedUserContext in your comments.
If this exists, it is also not exposed through the typings.

Since ITelemetryContext.user is NOT readonly in the typings, and the object I assigned to this property implemented the expected interface (IUserContext) (in version 2.5.8), I would argue that it was your typings that led me into doing what you tell me I should never do. According to the typings, what I did was perfectly fine.

I encourage you to tighten up the typings a bit and add readonly to properties/fields in the interfaces when they are meant to be readonly - to prevent others from doing things they should never do.

Thanks for your help!

@MSNev
Copy link
Collaborator

MSNev commented Apr 23, 2021

I encourage you to tighten up the typings a bit and add readonly to properties/fields in the interfaces when they are meant to be readonly - to prevent others from doing things they should never do.

Agree, there is a bunch of things that are slowly being addressed including the previously missing xxxAuthenticatedUserContext functions on the user object.

missing ai.setAuthenticatedUserContext

Will need to check this as it "should be" as it's defined here, it's probably got to do with the dynamic nature of the snippet as it can be 1 of 3 different implementations.

I'll create am issue for the typings...

@MSNev
Copy link
Collaborator

MSNev commented Apr 23, 2021

Created #1546

@josundt
Copy link
Author

josundt commented Apr 23, 2021

@MSNev Great, thanks 👍😃

@MSNev
Copy link
Collaborator

MSNev commented Jun 10, 2021

v2.6.3 is now fully deployed

@MSNev MSNev closed this as completed Jun 10, 2021
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants