Add better support for referencing global objects from a window and web workers #1167
Conversation
| Assert.ok(false, e.toString()); | ||
|
|
||
| completed = true; |
There was a problem hiding this comment.
Is this completed duplicated with this._testCompleted? 🤔
There was a problem hiding this comment.
No _testCompleted is a method, this is a flag to indicated that no additional steps should be processed.
MSNev
force-pushed
the
MSNev/AddGlobalThisSupport
branch
from
9eb48f2
to
f1d2633
Compare
| msg += "\n" + idx + ":" + value; | ||
| }); | ||
| } | ||
| Assert.ok(false, msg); |
There was a problem hiding this comment.
Why is msg false, isn't it always has a value?
There was a problem hiding this comment.
So that the message is always logged into the output, as this is only called when the tests fail.
MSNev
force-pushed
the
MSNev/AddGlobalThisSupport
branch
2 times, most recently
from
3d7bf2d
to
cf36ab7
Compare
MSNev
force-pushed
the
MSNev/AddGlobalThisSupport
branch
from
cf36ab7
to
b019107
Compare
| setTimeout(() => { | ||
| trigger(); | ||
| }, testInfo.stepDelay); | ||
| if (!completed) { |
There was a problem hiding this comment.
This file (and others) is shared between all other extensions and packages in this repo. In a separate PR we should probably share all of these files between all of them in case we ever need these changes in another package.
| @@ -597,7 +603,7 @@ export class ApplicationInsights extends BaseTelemetryPlugin implements IAppInsi | |||
| instance._onerror({ | |||
| message: error.reason.toString(), | |||
| error: error.reason instanceof Error ? error.reason : new Error(error.reason.toString()), | |||
| url: _window.location.href, | |||
| url: _location ? _location.href : "", | |||
There was a problem hiding this comment.
Was this causing issues before? This is changed behavior, but I don't think its preferred to send empty string here anyways.
| const originalFetch: (input?: Request | string, init?: RequestInit) => Promise<Response> = _window.fetch; | ||
|
|
||
| // Getting the global instance tp support web workers (which don't have window) | ||
| let global = getGlobal(); |
There was a problem hiding this comment.
This should be named _global?
| @@ -128,10 +213,8 @@ export function hasJSON(): boolean { | |||
| */ | |||
| export function getJSON(): JSON | null { | |||
| if (hasJSON()) { | |||
| return JSON; | |||
| return JSON || getGlobalInst(strJSON); | |||
There was a problem hiding this comment.
Is the JSON || part here still necessary?
No description provided.