[PWA-586] Country/Region Occasionally Resets initialValue #2456

tjwiebell · 2020-06-03T19:43:05Z

Description

There's an empty cache state for these components where the form they are attached to renders before the backend results for the list of available options in these dropdown components is returned. When that backend response arrives, the components re-render with those options, but the initialValue from the parent context doesn't apply, and the component renders an empty selection. I've more commonly seen this in the Region component, where State will be cleared out after an initial flash of the component.

Once this component has loaded once in the app, it's not reproducible, since the components first render now uses that immediate cached response.

Repro:

Add Simple item to cart and go to /cart
Open Estimate Shipping section and submit an estimate (State = TX)
Open Dev Tools > Local Storage > Delete apollo-cache-persist entry
Refresh the page and open Estimate Shipping section

Expected: State data entered is selected in dropdown
Actual: State is empty

Related Issue

[PWA-586] Country/Region Occasionally Resets initialValue

Acceptance

Verification Stakeholders

Specification

Verification Steps

⬆️ Repro steps can be used for verification above ⬆️

Screenshots / Screen Captures (if appropriate)

Checklist

I have updated/added tests to cover my changes, if necessary.

I have updated the documentation accordingly, if necessary.

…tions come back.

PWAStudioBot · 2020-06-03T19:55:35Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-586.

Generated by 🚫 dangerJS against ab0adc9

devops-pwa-codebuild · 2020-06-03T20:03:45Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2456.pwa-venia.com : LH Performance Expected 0.85 Actual 0.52, LH Best Practices Expected 1 Actual 0.92
https://pr-2456.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.36, LH Best Practices Expected 1 Actual 0.92
https://pr-2456.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.5, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

revanth0212

Technically speaking loading is the only change right? At first, I thought you removed the initialValue and validate props but they are fed into the same component inside ...inputProps or ...regionProps. Nice.

Would it be possible to add snapshot tests to the country and region components?

tjwiebell · 2020-06-04T17:06:59Z

Technically speaking loading is the only change right? At first, I thought you removed the initialValue and validate props but they are fed into the same component inside ...inputProps or ...regionProps. Nice.

Using loading to disable the field while fetching options + the magical keepState prop that preserves the value between mounts. This might even fix the issue you had in scope of billing address, where you were passing initalValue to the field instead of the form (this bug might have been why you did that).

Would it be possible to add snapshot tests to the country and region components?

Absolutely! Since we do tests in QA, is this code approved to get started on those?

revanth0212

Approved. Awaiting snapshot tests for Country and Region components.

tjwiebell · 2020-06-05T00:56:09Z

@revanth0212 - Added tests that are a bit coupled to these form input components, but wanted to put out a challenge, maybe you've solved this problem before. I really really tried to mock the Field and Select components in these tests, and could not get it to work. With the following mocks, I was getting an unhelpful PrettyFormatPluginError: val.getMockName is not a function error.

jest.mock('../../Field', () => props => <mock-Field {...props} />);
jest.mock('../../Select', () => 'Select');

I don't know what's different about the Select component, but it seems impossible to mock without getting this error.

revanth0212 · 2020-06-05T15:40:02Z

@revanth0212 - Added tests that are a bit coupled to these form input components, but wanted to put out a challenge, maybe you've solved this problem before. I really really tried to mock the Field and Select components in these tests, and could not get it to work. With the following mocks, I was getting an unhelpful PrettyFormatPluginError: val.getMockName is not a function error.
jest.mock('../../Field', () => props => <mock-Field {...props} />);
jest.mock('../../Select', () => 'Select');
I don't know what's different about the Select component, but it seems impossible to mock without getting this error.

Select is a component but you are returning a string. I guess that is the error. You are returning a component in case of Field though.

…eatment to Country.

dpatil-magento · 2020-06-09T17:28:40Z

QA Pass.

revanth0212

Thanks for adding tests @tjwiebell. A couple of comments, you can ignore them if you feel you have addressed those already.

revanth0212 · 2020-06-09T17:59:08Z

packages/peregrine/lib/talons/Region/__tests__/useRegion.spec.js

+        tree.update(<Component {...props} />);
+    });
+
+    expect(mockSetValue).toHaveBeenCalledWith();


Suggested change

expect(mockSetValue).toHaveBeenCalledWith();

expect(mockSetValue).toHaveBeenCalled();

Changed in ab0adc9.

revanth0212 · 2020-06-09T18:07:50Z

packages/peregrine/lib/talons/Region/useRegion.js


    const { data, error, loading } = useQuery(getRegionsQuery, {
        variables: { countryCode: country }
    });

-    let formattedRegionsData = [];
+    let formattedRegionsData = [{ label: 'Loading Regions...', value: '' }];


Do we have a test for this? Looks like it is being manipulated in multiple places. A set of tests can be of help.

I see you have added snapshot tests but more verbal ones can be helpful.

Added in ab0adc9

jimbo · 2020-06-09T18:40:02Z

packages/peregrine/lib/talons/Region/useRegion.js

+    useEffect(() => {
+        if (country) {
+            if (hasInitialized.current) {
+                regionFieldApi.setValue();


Maybe indicate this is a reset with a comment.

reset() from the fieldApi had the behavior I wanted, so I made that change in ab0adc9, which should be more self documenting. Thanks for pointing out there was much more to that API than the docs lead you to believe.

dpatil-magento · 2020-06-10T19:24:39Z

Re-verified after changes, looks good.

Keep state between renders so selected value doesn't get lost once op…

56c7b2c

…tions come back.

tjwiebell added the version: Patch This changeset includes backwards compatible bug fixes. label Jun 3, 2020

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Jun 3, 2020

revanth0212 requested changes Jun 4, 2020

View reviewed changes

m2-community-project bot assigned revanth0212 Jun 4, 2020

m2-community-project bot added the Progress: changes requested label Jun 4, 2020

revanth0212 previously approved these changes Jun 4, 2020

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: changes requested labels Jun 4, 2020

Add tests for region and country components

99ddf85

tjwiebell dismissed revanth0212’s stale review via 99ddf85 June 5, 2020 00:33

m2-community-project bot added Progress: review and removed Progress: approved labels Jun 5, 2020

Merge branch 'develop' into tommy/country-region-reset

ec9fde1

revanth0212 previously approved these changes Jun 5, 2020

View reviewed changes

Reimplement value reset

08b6f0d

tjwiebell dismissed revanth0212’s stale review via 08b6f0d June 8, 2020 17:37

tjwiebell closed this Jun 8, 2020

tjwiebell reopened this Jun 8, 2020

dpatil-magento and others added 3 commits June 8, 2020 14:03

Merge branch 'develop' into tommy/country-region-reset

0cdeee9

Remove keepState prop causing weird behavior, replace with similar tr…

b8e3f6c

…eatment to Country.

Merge branch 'develop' into tommy/country-region-reset

31b2ec6

revanth0212 previously approved these changes Jun 9, 2020

View reviewed changes

jimbo previously approved these changes Jun 9, 2020

View reviewed changes

m2-community-project bot assigned jimbo Jun 9, 2020

m2-community-project bot added Progress: approved and removed Progress: review labels Jun 9, 2020

dpatil-magento and others added 2 commits June 9, 2020 13:49

Merge branch 'develop' into tommy/country-region-reset

308981a

Use reset from fieldApi and update/add tests.

ab0adc9

tjwiebell dismissed stale reviews from jimbo and revanth0212 via ab0adc9 June 9, 2020 21:09

m2-community-project bot assigned tjwiebell Jun 9, 2020

m2-community-project bot added Progress: review and removed Progress: approved labels Jun 9, 2020

revanth0212 approved these changes Jun 10, 2020

View reviewed changes

jimbo approved these changes Jun 10, 2020

View reviewed changes

dpatil-magento merged commit 77d97c6 into develop Jun 10, 2020

dpatil-magento deleted the tommy/country-region-reset branch June 10, 2020 19:58

m2-community-project bot removed the Progress: review label Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PWA-586] Country/Region Occasionally Resets initialValue #2456

[PWA-586] Country/Region Occasionally Resets initialValue #2456

tjwiebell commented Jun 3, 2020 •

edited

Loading

PWAStudioBot commented Jun 3, 2020 •

edited

Loading

devops-pwa-codebuild commented Jun 3, 2020 •

edited

Loading

revanth0212 left a comment •

edited

Loading

tjwiebell commented Jun 4, 2020

revanth0212 left a comment

tjwiebell commented Jun 5, 2020 •

edited

Loading

revanth0212 commented Jun 5, 2020

dpatil-magento commented Jun 9, 2020

revanth0212 left a comment

revanth0212 Jun 9, 2020

tjwiebell Jun 9, 2020

revanth0212 Jun 9, 2020

tjwiebell Jun 9, 2020

jimbo Jun 9, 2020

tjwiebell Jun 9, 2020

dpatil-magento commented Jun 10, 2020

	expect(mockSetValue).toHaveBeenCalledWith();
	expect(mockSetValue).toHaveBeenCalled();

[PWA-586] Country/Region Occasionally Resets initialValue #2456

[PWA-586] Country/Region Occasionally Resets initialValue #2456

Conversation

tjwiebell commented Jun 3, 2020 • edited Loading

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jun 3, 2020 • edited Loading

devops-pwa-codebuild commented Jun 3, 2020 • edited Loading

revanth0212 left a comment • edited Loading

Choose a reason for hiding this comment

tjwiebell commented Jun 4, 2020

revanth0212 left a comment

Choose a reason for hiding this comment

tjwiebell commented Jun 5, 2020 • edited Loading

revanth0212 commented Jun 5, 2020

dpatil-magento commented Jun 9, 2020

revanth0212 left a comment

Choose a reason for hiding this comment

revanth0212 Jun 9, 2020

Choose a reason for hiding this comment

tjwiebell Jun 9, 2020

Choose a reason for hiding this comment

revanth0212 Jun 9, 2020

Choose a reason for hiding this comment

tjwiebell Jun 9, 2020

Choose a reason for hiding this comment

jimbo Jun 9, 2020

Choose a reason for hiding this comment

tjwiebell Jun 9, 2020

Choose a reason for hiding this comment

dpatil-magento commented Jun 10, 2020

tjwiebell commented Jun 3, 2020 •

edited

Loading

PWAStudioBot commented Jun 3, 2020 •

edited

Loading

devops-pwa-codebuild commented Jun 3, 2020 •

edited

Loading

revanth0212 left a comment •

edited

Loading

tjwiebell commented Jun 5, 2020 •

edited

Loading