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

Fix issues and react warnings for verification code components #1007

Merged
merged 10 commits into from
Sep 15, 2021

Conversation

eason9487
Copy link
Member

@eason9487 eason9487 commented Sep 10, 2021

Changes proposed in this Pull Request:

Closes #1002, #1003, #1004

  • 305c21c - Fix that the first verification code input isn't cleared after reset.
  • 2188f60 - Fix that backspace doson't remove selected digit in the <VerificationCodeControl>.
  • 49371b5 - Fix the warning of updating state on an unmounted component.
  • 8f87341 - Fix the warning of missing unique key in the <AppButton>.

Additional notes

  • 📝 Revert f605132 to removed <DemoVerifyPhoneNumberCard> before merging this PR.

Screenshots:

#1002 - Clear input after reset

Kapture 2021-09-10 at 15 19 51

#1003 - Delete selected value when pressing backspace

Kapture 2021-09-10 at 15 16 42

#1004 - Fix react warnings

Kapture 2021-09-10 at 15 54 21

Detailed test instructions:

  1. Go to the Setting page and there's a demo component for testing
  2. Follow the replication steps in each issue to check if they were fixed

Changelog entry

*
* @return {() => boolean} A function returns a boolean indicates the caller component is mounted or unmounted.
*/
export default function useIsMounted() {
Copy link
Member

Choose a reason for hiding this comment

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


Looks like native Node.isConnected. I'm surprised that React does not have it somewhat built-in and we have to hand-craft it? Do we do something not popular/standard here?

Copy link
Member Author

@eason9487 eason9487 Sep 15, 2021

Choose a reason for hiding this comment

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

I'm surprised that React does not have it somewhat built-in and we have to hand-craft it?

It had had a similar API isMounted, but was deprecated. https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html

Do we do something not popular/standard here?

It is difficult to say whether this practice is popular/standard or not because it's not really an issue or bug in most cases when seeing that warning.

useIsMounted is here mainly to avoid that scary warning appearing, but in fact there's no memory leak problem nor additional side effects.

The warning has quite a long background, mainly because some earlier global store/state management libraries based on the "Flux Architecture" had subscription and unsubscription mechanisms to be plugged in the class components. As an observer to get the global state changes, if a component forgets to unsubscribe after unmounted, it might let the global store keep the reference of the component itself or the method declared within the component, which may further lead to memory leaks.

However, with the implementation of something like react-redux, these subscription processes have been replaced by Higher-Order Components (HOC) and the memory leak problem is no longer common. After react hooks, these warnings are also almost false positives alarm. As it turns out, this warning will be adjusted in the new version of react, and we will no longer need useIsMounted then.

Comment on lines 113 to 136
/**
* Since the <InputControl> has an internal state management that always controls the actual `value` prop the <input>,
* the <InputControl> is forced to be a controlled input.
* When using it, it's always necessary to specify `value` prop from the below <AppInputControl>
* to avoid the warning - A component is changing an uncontrolled input to be controlled.
*
* @see https://github.com/WordPress/gutenberg/blob/%40wordpress/components%4012.0.8/packages/components/src/input-control/input-field.js#L47-L68
* @see https://github.com/WordPress/gutenberg/blob/%40wordpress/components%4012.0.8/packages/components/src/input-control/input-field.js#L115-L118
*
* But after specifying the `value` prop,
* the synchronization of external and internal `value` state will depend on whether the input is focused.
* It'd sync external to internal only if the input is not focused.
* So here we await the `digits` is reset back to `initDigits` by above useEffect and sync to internal value,
* then move the focus calling after the synchronization tick finished.
*
* @see https://github.com/WordPress/gutenberg/blob/%40wordpress/components%4012.0.8/packages/components/src/input-control/input-field.js#L73-L90
*
* And it's also used for the first time focus after this component being mounted.
*/
useEffect( () => {
if ( digits === initDigits ) {
maybeMoveFocus( 0 );
}
}, [ resetNeedle, digits ] );
Copy link
Member

Choose a reason for hiding this comment

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

👍 Thanks for the detailed explanations :) It makes it clear why the code is here not where it was before.
💅 However, I think we could also add a note of why we need it at all/what it does, for somebody who reads the file as is, not as a change to the previous state.

I made a branch with a small proposal fix/1002-1003-1004-verification-code...fix/1002-1003-1004-verification-code-comments, feel free to merge it if you like it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Merged. Thanks!

Copy link
Member

@tomalec tomalec left a comment

Choose a reason for hiding this comment

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

Reviewed the code, tested locally on WP58 & WC57, LGTM.
Left few comments about cosmetics.

Thanks for the demo component, and detailed explanations.

@eason9487 eason9487 merged commit 5e7f1ee into develop Sep 15, 2021
@eason9487 eason9487 deleted the fix/1002-1003-1004-verification-code branch September 15, 2021 05:12
@eason9487 eason9487 added this to the 1.5 milestone Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants