-
-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
Signed-off-by: Michael Telatynski <[email protected]>
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 very happy to see a fix to this flake, but can you talk me through how this helps?
The JSDom CSSDeclaration stuff doesn't behave exactly the same as it does in browsers. Namely the object has a bunch of extra fields which don't match the types we come to expect from the signature, so changing the way we iterate it fixes that. There was then another flake where we're rendering MatrixChat without the bits of element-web's index.html which set up styles which caused a race condition with the ThemeWatcher firing off other bits of theming code which were unhappy to be missing the theme bits in index.html |
@@ -60,6 +60,7 @@ import { UIFeature } from "../../../src/settings/UIFeature"; | |||
jest.mock("matrix-js-sdk/src/oidc/authorize", () => ({ | |||
completeAuthorizationCodeGrant: jest.fn(), | |||
})); | |||
jest.mock("../../../src/settings/watchers/ThemeWatcher"); |
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.
There was then another flake where we're rendering MatrixChat without the bits of element-web's index.html which set up styles which caused a race condition with the ThemeWatcher firing off other bits of theming code which were unhappy to be missing the theme bits in index.html
maybe add a comment here to explain that, and say we're disabling ThemeWatcher?
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.
Done
src/theme.ts
Outdated
for (let i = 0; i < document.body.style.length; i++) { | ||
const prop = document.body.style[i]; |
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.
The JSDom CSSDeclaration stuff doesn't behave exactly the same as it does in browsers. Namely the object
"the object" here being document.body.style
? Which is supposed to be a CSSStyleDeclaration but maybe isn't?
has a bunch of extra fields which don't match the types we come to expect from the signature, so changing the way we iterate it fixes that.
Ok, but:
- why was it intermittent?
- Again, a comment here explaining that we're working around jsdom weirdness would be good. The first thing I want to do when I see this code is refactor it to
for (const prop of document.body.style) { ... }
, which for any normal array-like-object would be identical.
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.
why was it intermittent?
Race condition between the test & ThemeWatcher
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.
This part was inherently just defensive against JSDom but should be superfluous given the stubbing out of ThemeWatcher
Signed-off-by: Michael Telatynski <[email protected]>
Fixes element-hq/element-web#28122