Fix/1078 shipping values flash during onboarding setup

[jorgemd24]

Changes proposed in this Pull Request:

Closes #1078

What was causing the issue?

The issue was caused because:

1. When the Shipping Rate/Shipping Times field is updated, it triggers the API to save the values in the server.
2. Once the values are saved and the request is finished, the store is updated.
3. Each input (Shipping Rates & Time Rates) has a useEffect with the following format:

const [ value, setValue ] = useState( savedValue );

useEffect( () => {
	setValue( savedValue );
	}, [ savedValue ] );

//Sample of savedValue = { countries:["UK"..] , price: 1 }

It should only update the state if the savedValue is updated. I did some debugging and I see that React detects that savedValue is different every time therefore is creating extras re-renderings in both components. This causes that the input gets re-render always with the savedValue instead of the value from the state, consequently showing old data.

Proposed solution

This has been resolved updating the useEffect function with the following format:

useEffect( () => {
	setValue( savedValue );
}, [ savedValue.price ] );

useEffect( () => {
	setValue( savedValue );
}, [ savedValue.time ] );

In this way we make sure that it should only update the state with the savedValues if the price or time has been updated.

I think we should check other components that can have the same issue and could impact the performance.

With quicker connections, the same issue was happening but it was not visible for the user.

Screenshots:

Detailed test instructions:

Visual Testing

1. Go to step 2 of onboarding setup page.
2. Select "All countries" for audience location.
3. Move to step 3
4. Open browser DevTool and throttle network with "Slow 3G"

Unit Tests:

1. npm run test:js -- countriesTimeInput
2. npm run test:js -- countriesPriceInput

[ecgan]

Just a quick non-thorough review to add two 🚧 blocker comments below.

[ecgan]

🚧 Same thing, React ESLint will complain about this: "React Hook useEffect has a missing dependency: 'savedValue'. Either include it or remove the dependency array.eslintreact-hooks/exhaustive-deps".

[puntope]

Not judging, just curious!

Why the GH Actions linter is not failing? In theory the GH actions and the linter task should help us to handle this if it's a blocker.

[eason9487]

There's the --quiet option in this workflow to skip the JS linter warnings. Otherwise, it would show 14 lint warnings by GH annotations in every PR. We could remove the --quiet if it's more helpful than the current.

[puntope]

Is not there a way to promote "React Hook useEffect has a missing dependency... as error? Since it's a blocker issue. Or on the other side, add annotation somewhere in order to ignore the other warnings and remove --quiet

[eason9487]

Is not there a way to promote "React Hook useEffect has a missing dependency... as error?

It can be done by changing 'warn' to 'error' here:

google-listings-and-ads/.eslintrc.js

Lines 14 to 15 in cf03ab5

	'react-hooks/exhaustive-deps': [
	'warn',

[puntope]

Thanks for adding tests.

❓

Is there a reason to have countriesPriceInput.test.js and countriesTimeInput.test.js in separated files?

All the mocks and external deps are duplicated between both files. I'd consider to use only one file tests/countriesInput.test.js and test there both.

Besides the dependencies , the tests themselves are kind of duplicated since both are using same logic

[jorgemd24]

The idea was to keep different files to make easier the addition of other specific tests for each component. If you like I could join both tests in one file and if we need to add more tests to these components in the future, we could:

• Continuing adding the tests in the same file
• Or create a new file depending on the logic and component that we are going to test.

[puntope]

Didn't check all the code yet but I can see how add-rate-modal/useGetRemainingCountryCodes and add-time-modal/useGetRemainingCountryCodes are duplicated code (only one line changes between them, and name is same actually), same goes with all the files in the modals (add & edit) and maybe more... (TBD)

This is totally out scope of this issue but feels for me that a refactor is needed here.

Since I don't wanna block the PR and this is more likely a generalised problem in the component I will open a separated issue regarding the refactor (Including this tests).

Thoughts?

[jorgemd24]

I agree that shippings-rates & shipping-times are components that seem to have the same logic, consequently, duplicating code and logic. Let's open the issue where we can discuss if it is needed to refactor those two components. Thanks!

[ecgan]

@jorgemd24 , I tested your solution in this PR here, the solution somewhat helps the issue when users change the values slowly. If they change the values back and forth quickly over slow connections, then we would still see flashing of old values.

📹 See my demo video below with my voice annotation:

shipping-rates-values-flashing.mov

I think your solution is a good enough as it does somewhat help the issue (reduce the chances of users seeing flashing of old values). To fix the issue completely, I think we would need to work on it in #288 which would require quite a bit of time (not really a "good first issue" as indicated in #1078). I hope I can work on it soon one day.

[ecgan]

Tested, the solution somewhat helps the issue #1078 but not completely, see my demo video in #1157 (comment). All tests passed. 👍

Given that it somewhat helps the issue for now, I think we are good to go with this PR. For the long term, I think we should look into working on #288.

[jorgemd24]

Tested, the solution somewhat helps the issue #1078 but not completely, see my demo video in #1157 (comment). All tests passed. 👍

Given that it somewhat helps the issue for now, I think we are good to go with this PR. For the long term, I think we should look into working on #288.

Thanks, @ecgan, I agree with you that this is a temporal solution. I am sure that with the implementations from #288 we are going to tackle the root of the problem.