fix(material/core): sanity checks not disabled for node-based test environments #23636
Conversation
crisbeto
added
P2
google-cla
bot
added
the
cla: yes
There was a problem hiding this comment.
LGTM. have you also considered using https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#description?
crisbeto
force-pushed
the
23365/sanity-checks-test-again
branch
from
8e20836 to
81e467d
Compare
crisbeto
force-pushed
the
23365/sanity-checks-test-again
branch
from
81e467d to
be6774f
Compare
| @@ -99,10 +99,10 @@ export class FocusMonitor implements OnDestroy { | |||
| private _windowFocused = false; | |||
|
|
|||
| /** The timeout id of the window focus timeout. */ | |||
| private _windowFocusTimeoutId: number; | |||
| private _windowFocusTimeoutId: any; | |||
There was a problem hiding this comment.
These are necessary, because the type now is number | NodeJS.Timer.
There was a problem hiding this comment.
Hmm. looks like these any casts are just needed for the legacy Saucelabs setup. Should we instead just declare the global type directly instead? I think that would be better than giving up on type safety. e.g.
declare const global: any;There was a problem hiding this comment.
There's a comment at the top of that file about avoiding declare const due to an issue with Closure so that's why I wrote it this way in the first place. Otherwise the same could've been written as declare cost jest: any.
There was a problem hiding this comment.
oh that's unfortunate. What is about globalThis? this should work as well and not need any Node types. Seems like it's ideal for such platform-agnostic checks
There was a problem hiding this comment.
I avoided it since it isn't supported in some browsers (not sure if we care about them) and the workaround wasn't that much more code.
There was a problem hiding this comment.
okay, I don't feel very strongly about it. It seems supported by all of the browsers we make guarantees for (https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/globalThis), and it would save us from using any in all of cdk/ (for the timers).
crisbeto
force-pushed
the
23365/sanity-checks-test-again
branch
from
be6774f to
100028e
Compare
…vironments In angular#23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the `window` which won't work in a Node environment, because the global variables are attached to the `global` object, not the `window`, even though there may be a fake `window` declared by the test tooling. These changes resolve the issue by first checking against `global` before falling back to `window`. Fixes angular#23365.
crisbeto
force-pushed
the
23365/sanity-checks-test-again
branch
from
100028e to
52cba8d
Compare
|
Thanks for the follow-through on this. |
…vironments (#23636) In #23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the `window` which won't work in a Node environment, because the global variables are attached to the `global` object, not the `window`, even though there may be a fake `window` declared by the test tooling. These changes resolve the issue by first checking against `global` before falling back to `window`. Fixes #23365. (cherry picked from commit 4f6b9fd)
In angular#23636 the test environment check was changed so that it looks for the test objects either on `window` or `global`, however it looks like this isn't enough to pick up Jest which isn't published on either. These changes simplify the setup by looking up the value globally and disabling the type checkng error with `@ts-ignore`. We can't use `declare const` for it, because it causes issues in g3. I've also reverted some of the `any` types that had to be added in angular#23636. Fixes angular#23365.
In angular#23636 the test environment check was changed so that it looks for the test objects either on `window` or `global`, however it looks like this isn't enough to pick up Jest which isn't published on either. These changes simplify the setup by looking up the value globally and disabling the type checkng error with `@ts-ignore`. We can't use `declare const` for it, because it causes issues in g3. I've also reverted some of the `any` types that had to be added in angular#23636. Fixes angular#23365.
In angular#23636 the test environment check was changed so that it looks for the test objects either on `window` or `global`, however it looks like this isn't enough to pick up Jest which isn't published on either. These changes simplify the setup by looking up the value globally and disabling the type checkng error with `@ts-ignore`. We can't use `declare const` for it, because it causes issues in g3. I've also reverted some of the `any` types that had to be added in angular#23636. Fixes angular#23365.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
In #23374 we expanded the logic that checks for test environments to cover Jest and Mocha. The problem is that we were checking against the
windowwhich won't work in a Node environment, because the global variables are attached to theglobalobject, not thewindow, even though there may be a fakewindowdeclared by the test tooling.These changes resolve the issue by first checking against
globalbefore falling back towindow.Fixes #23365.