fix(material/core): sanity checks not disabled for node-based test environments - #23636
Conversation
Sorry, something went wrong.
devversion
left a comment
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?
Sorry, something went wrong.
|
|
||
| /** 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.
Sorry, something went wrong.
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;
Sorry, something went wrong.
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.
Sorry, something went wrong.
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
Sorry, something went wrong.
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.
Sorry, something went wrong.
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).
Sorry, something went wrong.
…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.
|
Thanks for the follow-through on this. |
Sorry, something went wrong.
…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. |
Sorry, something went wrong.
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.