Refactor useMediaQuery with useSyncExternalStore #48973
Conversation
|
Size Change: +1.05 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Mamaduka
added
[Type] Enhancement
|
I'm unsure how to emulate/mock the event-driven behavior for Maybe tests like these are better suited for the E2E/Playwright. Refactoring becomes more challenging if I mock stub/everything and we end up testing implementation details. @jsnajdr, @tyxla, @kevin940726, any suggestion on how I can improve tests for this hook? |
|
IMO, if the test is just testing implementation details then it's not providing much value. I'd vote for removing it if it's too much trouble to fix. |
The whole test suite for this hook is like that. We're mocking the return values of |
Mamaduka
changed the title
The const { container } = render( <TestComponent query="(min-width: 782px)" /> );
// the query doesn't match yet
expect( container ).toHaveTextContent( 'useMediaQuery: false' );
// make the query match
act( () => {
window.matchMedia.useMediaQuery( '(min-width: 782px)' );
} );
// test that it has rerendered
expect( container ).toHaveTextContent( 'useMediaQuery: true' );With |
|
@jsnajdr, for some reason, that also fails. I'm using this logic to test if listeners are removed. gutenberg/packages/compose/src/hooks/use-media-query/test/index.js Lines 37 to 40 in 954b7ef |
What exactly fails? Looking at the https://github.com/dyakovk/jest-matchmedia-mock/blob/master/src/matchmedia-mock.ts#L31 After So, yes, in the test I suggested above, after the Another thing that looks bad is that the mock doesn't fire an event with I think we'd be better off writing our own simple mock. |
package.json
Outdated
| @@ -197,6 +197,7 @@ | |||
| "jest": "29.5.0", | |||
| "jest-jasmine2": "29.5.0", | |||
| "jest-junit": "13.0.0", | |||
| "jest-matchmedia-mock": "1.1.0", | |||
There was a problem hiding this comment.
Should it be a dev dependency of the compose package though? Is there a good reason why it's required by the entire project?
There was a problem hiding this comment.
Probably, but the sub-packages don't have devDependencies specified, so I put it here. In any case, we'll go with custom mocking for the tests - #48973 (comment).
There was a problem hiding this comment.
I agree with Jarda on using custom mockups for it 👍 Seems like a sensible and flexible enough solution for the time being.
There was a problem hiding this comment.
Should it be a dev dependency of the compose package though?
I think that a few years ago, we had serious issues with devDependencies being declared on individual packages. Something with Lerna, or hoisting, or was it NPM unable to find a binary for such a package, in the top-level or package-specific node_modules/.bin directory? I don't remember, maybe @blowery or @sirreal does.
In any case, Gutenberg seems to follow a convention where all devDependencies are declared at the top. While Calypso prefers that individual packages have their own devDependencies. Many of the were added during the Yarn migration though.
There was a problem hiding this comment.
Sorry, I don't remember the details about these conventions.
|
Thanks again, @jsnajdr! I'll try to come up with a simple mock for these tests later today or early next week. P.S. Here's the current test's failure.
|
|
@Mamaduka in case that's helpful, in Automattic/wp-calypso#73890 I recently mocked Perhaps you could reuse it with some adaptation to suit your needs? |
It's like if someone already called |
|
I've found a different library ( P.S. I tried writing a "simple mock", but emulating events was more challenging than I expected. |
|
The PR is ready for testing 🙇 |
| addListener = jest.fn(); | ||
| removeListener = jest.fn(); | ||
| window.matchMedia = matchMedia; | ||
| window.MediaQueryListEvent = MediaQueryListEvent; |
There was a problem hiding this comment.
Do you need to set the MediaQueryListEvent global? That seems useful only if you wanted to manually create and dispatch these events.
There was a problem hiding this comment.
I need to check and confirm, but this is based on the library's Jest setup polyfill - https://github.com/Ayc0/mock-match-media/blob/master/polyfill.js.
I'll remove it if it's not required to trigger changes.
Update: You were right; setting the MediaQueryListEvent isn't needed.
That's probably OK, because big part of the work is done by |
|
Flaky tests detected in a0b0cb9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4428026786
|
What?
PR refactors
useMediaQueryhook to useuseSyncExternalStoreinstead ofuseEffect.Why?
The
useSyncExternalStoreis better suited for similar subscriptions.Testing Instructions
Screenshots or screencast
CleanShot.2023-03-15.at.13.31.33.mp4