Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add useViewportMatch react hook alternative to withViewportMatch #18816

Merged
merged 6 commits into from Dec 5, 2019

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Nov 29, 2019

In an effort to flatten the React Tree, I've added a useViewportMatch to the compose package. It relies on the existing useMediaQuery similar to useReducedMotion.

Pros

One nice side-effect is that this removes the dependency to @wordpress/viewport which itself depends on @wordpress/data and we were always reluctant to use it the @wordpress/components package because of this. Right now, the dependency is not removed from the existing packages because we still have class components where we can't use hooks yet.

Cons

The potential downside is that the viewport package alwas trigger a fixed number of listerners, I think it's 10 or 12 listeners, while the number here depends on how much the hook is used. That said, adding a cache for useMediaQuery React hook is possible and it will be useful for all media query hooks, not just the viewport one.

Performance

In terms of performance, I did a comparison with master and I didn't notice any impact so far, I think once we remove the "viewport" package dependency entirely, we might notice a gain in performance.

Mobile

I'm not familiar with react-native enough to build the alternative mobile version as well, but if possible, it would be good to provide one there as well. cc @hypest

Copy link
Member

ellatrix left a comment

Really nice improvement.

@@ -393,16 +394,19 @@ function BlockListBlock( {

// If the block is selected and we're typing the block should not appear.
// Empty paragraph blocks should always show up as unselected.
const isFocusModeActive = isFocusMode && isLargeViewport;

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 29, 2019

Member

Nice, this is much clearer.

packages/compose/README.md Show resolved Hide resolved
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 29, 2019

Noting that this will probably make #5316 obsolete, cc @aduth.

@youknowriad youknowriad mentioned this pull request Dec 4, 2019
6 of 6 tasks complete
@youknowriad youknowriad force-pushed the add/use-viewport-match branch from 3c85f9e to dea8960 Dec 4, 2019
@hypest

This comment has been minimized.

Copy link
Contributor

hypest commented Dec 4, 2019

Mobile

I'm not familiar with react-native enough to build the alternative mobile version as well, but if possible, it would be good to provide one there as well. cc @hypest

@Tug , can you have a quick look and follow up here? We at least need to make sure we're not going down a path that'll be incompat with RN and, whether we can afford merging the web side while we will work on the native side in a separate PR.Thanks!

@Tug

This comment has been minimized.

Copy link
Contributor

Tug commented Dec 4, 2019

Concerning incompatibilities I think we're fine, if we look at the impacted components:

  • BlockMobileToolbar => We use a native version
  • ToolSelector => We don't use it
  • HeaderToolbar => We have a native version
  • PostPublishButtonOrToggle => We don't use it
  • WritingMenu => We don't use it
  • Layout => We have a native version
  • PostSwitchToDraftButton => We don't use it

Afaik we only use withViewportMatch for MediaText and this addition does not seem to introduce breaking changes on this HoC anyway

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 4, 2019

@Tug do you think it's possible to build a mobile version for the hook (not necessarily now) but eventually, the viewport package could be deprecated.

@Tug

This comment has been minimized.

Copy link
Contributor

Tug commented Dec 4, 2019

I don't think it should be too hard yeah, we would need a polyfill for window.matchMedia. I think it would be a generalization of @koke's work in #17682
Still, better do that in another PR 👍

@aduth
aduth approved these changes Dec 4, 2019
Copy link
Member

aduth left a comment

I left some ideas for you to consider, but this seems like a very sensible improvement over the viewport package/store/HoC.

On the point of performance: I think this will probably be fine. Not sure if there's really much overhead with MediaQueryList#addListener.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 4, 2019

An interesting advantage that higher-order components have over hooks is relevant for this, in that if a composition of higher-order components includes one which conditionally renders, it can prevent the logic of the next from being unnecessarily evaluated. This is in contrast to hooks, where all hooks must be called before any conditions are made about whether the component renders. It's especially notable for something like withSelect or useSelect, which are more prone to causing performance issues that we could avoid by simply not allowing those subscriptions to be attached if not applicable under certain conditions (viewport size, in this case).

That being said, I'm more in favor of being consistent in how we approach this, and I think hooks are probably the way we want to go for most everything. I expect it's probably worth the trade-off.

@youknowriad youknowriad force-pushed the add/use-viewport-match branch from 181f77a to fecbfaa Dec 5, 2019
@youknowriad youknowriad requested a review from chrisvanpatten as a code owner Dec 5, 2019
@youknowriad youknowriad merged commit f4300ff into master Dec 5, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Failed
Details
@youknowriad youknowriad deleted the add/use-viewport-match branch Dec 5, 2019
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 6, 2019

The potential downside is that the viewport package alwas trigger a fixed number of listerners, I think it's 10 or 12 listeners, while the number here depends on how much the hook is used. That said, adding a cache for useMediaQuery React hook is possible and it will be useful for all media query hooks, not just the viewport one.

Was this the reason for not replacing the use of withViewportMatch in BlockListBlock (which is otherwise already a function component capable of using the hook)?

withViewportMatch( { isLargeViewport: 'medium' } ),

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 6, 2019

Yeah, that's the main reason (I didn't measure though), there's another reason which BC of the filter, that said, I believe this is a prop that is not so important and that we can remove with a dev note.

@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 11, 2019

The potential downside is that the viewport package alwas trigger a fixed number of listerners, I think it's 10 or 12 listeners, while the number here depends on how much the hook is used. That said, adding a cache for useMediaQuery React hook is possible and it will be useful for all media query hooks, not just the viewport one.

Was this the reason for not replacing the use of withViewportMatch in BlockListBlock (which is otherwise already a function component capable of using the hook)?

Yeah, that's the main reason (I didn't measure though)

I wonder about this actually. Depending on whether the browser internally normalizes multiple matchMedia event handlers of the same query, the only difference would be in whether the browser can notify subscribers more optimally than the store's own implementation. Given that this occurs in browser-land (C++), I'd be inclined to think it would. That being said, it would likely be contingent on how the browser treats "duplicate" matchMedia, or whether there's any overhead in this crossover between the native event emitter and the JavaScript runtime. tl;dr: Could be more performant, but needs measurements 😄

It reminds me of withGlobalEvents, which tries to handle this same idea on behalf of the browser in "binding at most a single event handler for the entire application". More recently, I've come to doubt that it really has any advantage over distinct event handlers.

And this doesn't even account for any benefits we might see simply by migrating from a higher-order component to a hook (tree flattening).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.