Add hooks and HOCs to lib/viewport

[sgomes]

Changes proposed in this Pull Request

• Fix a bug in listener removal in lib/viewport
• Add tests for lib/viewport
• Add helper hooks and higher-order components to lib/viewport/react-helpers.
• Add tests for lib/viewport/react-helpers hooks and HOCs.

Testing instructions

• Ensure that the modified components making use of the helpers still work. It may be difficult to trigger the reactive behaviour for some of them.
• Ensure the new tests pass.

CC @jsnajdr: I think I found a reasonable way of testing hook (and HOC) reactivity. What do you think?

[matticbot]

Test live: https://calypso.live/?branch=update/improve-lib-viewport

[blowery]

that's a global change though isn't it?

[sgomes]

Doesn't every file run separately in Jest?

[blowery]

just saying that this relies on jest isolating globals on test runs across each test file. is that a safe assumption?

[sgomes]

It does seem potentially dangerous, I agree. I'll find a better way of doing this.

[sgomes]

Done, using spies now.

[blowery]

thanks!

[jsnajdr]

just saying that this relies on jest isolating globals on test runs across each test file. is that a safe assumption?

I agree with doing the mocks as if the globals were not isolated, but it's safe to assume that they are. The global is sandboxed, provided by the jest-environment as configured globally or per-file, and recreated for each file.

Each Jest file can specify a pragma at the beginning:

/**
 * @jest-environment jsdom
 */

A rather unavoidable conclusion of this is that the global is different for each file, either Node-ish or browser-ish.

[sgomes]

All comments addressed, and several improvements made. Ready for another look.

[blowery]

LGTM, but I'd love @jsnajdr to look it over too.

[jsnajdr]

I'm not enthusiastic about the new addWithinBreakpointListener API. There are two common patterns for adding and removing listeners:

addListener and removeListener that relies on listener's identity
Common in DOM APIs and in Node.js, standardized as EventTarget:

const listener = () => { ... };
el.addEventListener( 'scroll', listener );
el.removeEventListener( 'scroll', listener );

To remove a listener, exactly the same listener object must be passed to the remove function. Both functions return void.

subscribe function that returns an unsubscribe function
Used in Redux (store.subscribe), in RxJS and in React.useEffect:

const unsubscribe = subscribe( listener );
// forget the listener, remember the unsubscribe function
unsubscribe();

Can we choose one of the patterns and stick to it? addWithinBreakpointListener is now a confusing mixture of both.

[sgomes]

I'm not enthusiastic about the new addWithinBreakpointListener API. There are two common patterns for adding and removing listeners:

addListener and removeListener that relies on listener's identity
Common in DOM APIs and in Node.js, standardized as EventTarget:

const listener = () => { ... };
el.addEventListener( 'scroll', listener );
el.removeEventListener( 'scroll', listener );

To remove a listener, exactly the same listener object must be passed to the remove function. Both functions return void.

subscribe function that returns an unsubscribe function
Used in Redux (store.subscribe), in RxJS and in React.useEffect:

const unsubscribe = subscribe( listener );
// forget the listener, remember the unsubscribe function
unsubscribe();

Can we choose one of the patterns and stick to it? addWithinBreakpointListener is now a confusing mixture of both.

You're right, it did get pretty confusing. I switched to the latter method, which neatly allows us to reduce the API surface and keep it more consistent 👍

[sgomes]

Thank you for the excellent review as usual, @jsnajdr! 👍

All comments addressed.

[sgomes]

New comments addressed as well. Thanks again, @jsnajdr !

[jsnajdr]

I can't find any more issues 😆

[sgomes]

I can't find any more issues 😆

You make that sound like a bad thing! 😃

Thanks again for the thorough review and all the back-and-forth, @jsnajdr! I can confidently say this was much more complex than I originally thought it would be... 🐇 🕳