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

Update the block component to use react hooks instead #14985

Merged
merged 5 commits into from
May 7, 2019

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Apr 15, 2019

I said previously that we're not going to refactor our components to use hooks just to use hooks. But in this PR, I'm trying to refactor the BlockListBlock component.

The main reasons are:

  • The component is one of the most complex one in the repository and using hooks clarifies its code as related behaviors/functions/state are grouped.
  • This is one of the bottleneck components in terms of performance, and I'd like to explore whether we can make improvements using hooks or not.
  • I have some plans to bring new features to this component and I don't want to just continue growing without some "organization".

At the moment, I kept the refactoring as minimal as I can, which means I kept, in theory, the same flows, states... There's something different though which can be impactful. A lot of the callbacks are not bound the instance object which means some child components might rerender more often. There are options and solutions but I want to measure the differences before making changes.

  • I can use useCallback to memoize these callbacks
  • I want to be mindful of the memory impact of memoizing all of these callbacks, maybe these are rerendered regardless of the callback change which means the memoization can be more costly than anticipated.
  • Another option could be to move some of these callbacks to the withDispatch HoC instead.

@youknowriad youknowriad added the [Type] Code Quality Issues or PRs that relate to code quality label Apr 15, 2019
@youknowriad youknowriad requested review from aduth and mtias April 15, 2019 12:40
@mtias
Copy link
Member

mtias commented Apr 15, 2019

This is a good start. Making this code more straightforward is going to help always keeping a cleaner state for any future iterations.

There's another motivation for making use of hooks here which is getting some more concrete awareness of how they can later benefit the consumable block API (exposing some of what has been so far absorbed in support or higher order components through hooks) and its tradeoffs.

@youknowriad
Copy link
Contributor Author

I run the performance tests to compare this branch against master and noticed similar times. This refactor has no impact on performance.

Can I get a review here, I'm thinking of merging this as is and try other smaller improvements using hooks separately.

@youknowriad youknowriad force-pushed the update/block-component-hooks branch 2 times, most recently from 79f7a6e to 453dc59 Compare April 17, 2019 12:07
@youknowriad youknowriad requested a review from a team April 17, 2019 12:09
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit difficult to review, but it appears in good shape. I like how hooks help to consolidate where previously we have disjointedness of checking for changes in props values between componentDidMount, componentDidUpdate.

The issue with the ESLint warning should probably be resolved. There's also a merge conflict.

@hypest
Copy link
Contributor

hypest commented May 7, 2019

@Tug , can you give this PR a quick look to see if it can affect the native mobile in any way? cc @youknowriad

@youknowriad youknowriad force-pushed the update/block-component-hooks branch from 62ad155 to 709cf53 Compare May 7, 2019 09:45
@Tug
Copy link
Contributor

Tug commented May 7, 2019

@hypest afaics it's only touching BlockListBlock which we're not using at the moment.
It might affect the way we port BlockHolder to gutenberg but it should not break anything right now.

@youknowriad youknowriad merged commit b66bbcc into master May 7, 2019
@youknowriad youknowriad deleted the update/block-component-hooks branch May 7, 2019 11:59
@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019
@@ -26,6 +26,5 @@ module.exports = {
'react/prop-types': 'off',
'react/react-in-jsx-scope': 'off',
'react-hooks/rules-of-hooks': 'error',
'react-hooks/exhaustive-deps': 'warn',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this was removed? Looking back through the history to #14995 (comment), I see there was some initial uncertainty about the rule. But as I've been reading more about it, it seems like one of those things where it should be a safe default to use, and that most problems end up from mis-use (related discussion). What are these valid exceptions we have, and are they really the majority of cases that we shouldn't only be opting-out as opposed to disabling this rule altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I first started trying hooks, it felt that we had a too many exceptions. Maybe related to the way our components were written or related to my thought process when writing components. happy to reconsider.

Copy link
Member

@aduth aduth Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truthfully, I'm still trying to understand all that the rule is meant to cover (its documentation... doesn't exist), but the general tone of the discussion around it from their end seems to be that if a developer is fighting with the rule, there's a higher likelihood the developer is "doing it wrong", so I'm curious to see a few specifics.

Maybe I'll turn on the rule locally to see what issues come up, in order to get a better understanding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It:

  • Forces you to pass deps = [].
  • Statically analyzes callbacks for closures and forces you to add them to deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants