This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Fix React hook dependency warnings in Cart & Checkout blocks + withAttributes HOC #3314
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Aljullu
added
status: needs review
type: refactor
|
Size Change: +421 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
senadir
approved these changes
Oct 23, 2020
There was a problem hiding this comment.
This is a great PR, unlike my comment here #3285 (review) I feel more confident merging this PR, it makes sense and no change require documentation because the diff is self-documenting
Comment on lines
64
to
80
| useEffect( () => { | ||
| if ( validateOnMount ) { | ||
| validateInput(); | ||
| if ( isPristine ) { | ||
| if ( focusOnMount ) { | ||
| inputRef.current.focus(); | ||
| } | ||
| if ( validateOnMount ) { | ||
| validateInput(); | ||
| } | ||
| setIsPristine( false ); | ||
| } | ||
| }, [ validateOnMount ] ); | ||
| }, [ | ||
| focusOnMount, | ||
| isPristine, | ||
| setIsPristine, | ||
| validateOnMount, | ||
| validateInput, | ||
| ] ); |
There was a problem hiding this comment.
Why did you merge two useEffects? the general conscious is to separate useEffects unless you have a reason?
There was a problem hiding this comment.
Good point. I undid the merge in 14c4ae8.
Comment on lines
-43
to
-49
| /** | ||
| * @todo Investigate extra POST request on initial cart render. | ||
| * | ||
| * We have an unfixed bug in our test in which the full cart triggers a POST | ||
| * request to `wc/store/cart/update-item` causing the fetch to be called twice. | ||
| */ | ||
| expect( fetchMock ).toHaveBeenCalledTimes( 2 ); |
There was a problem hiding this comment.
Should probably also close this
#2813
There was a problem hiding this comment.
Ah, yes. Thanks for pointing to the issue. I assigned it to me and linked it with this PR.
| @@ -3,7 +3,6 @@ | |||
| */ | |||
| import { useState, useEffect } from '@wordpress/element'; | |||
| import { getAttributes, getTerms } from '@woocommerce/editor-components/utils'; | |||
| import { find } from 'lodash'; | |||
assets/js/hocs/with-attributes.js
Outdated
| const { selected } = props; | ||
| const [ attributes, setAttributes ] = useState( [] ); | ||
| const { selected = [] } = props; | ||
| const selectedSlug = selected.length ? selected[ 0 ]?.attr_slug : null; |
There was a problem hiding this comment.
In my experience, when working with an expected data endpoint (like our own REST API), using optional chaining signals a code smell.
Disregarding line 32, do we expect selected to be an empty array?
Also if you defaulting to an empty array in line 32, length would evaluate to 0, which would skip directly to null, so no optional chaining required.
There was a problem hiding this comment.
Yeah... good point. I don't think the optional chaining is necessary at all, probably it's a left-over from an in-progress refactor I was doing. Fixed in bca57a5.
Aljullu
deleted the
refactor/fix-hook-dependency-warnings-cart-checkout
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #3204.
Related to #3285.
Fixes #2813.
How to test the changes in this Pull Request:
Cart and Checkout blocks
Most of the changes are related to input field validation/displaying errors + the cart item quantity input, so these are the main flows that need to be tested.
withAttributes HOC