From 94926b6345ed59b9f673287437236ac62939b134 Mon Sep 17 00:00:00 2001 From: sdrozdsap <163305268+sdrozdsap@users.noreply.github.com> Date: Tue, 2 Jul 2024 11:45:25 +0100 Subject: [PATCH] fix: Redundant reacreation of cart items form (#18962) --- .../cart-item-list.component.spec.ts | 52 ++++++++++++++----- .../cart-item-list.component.ts | 36 ++++++++++--- .../feature-toggles/config/feature-toggles.ts | 7 +++ .../spartacus/spartacus-features.module.ts | 1 + .../item-counter.component.spec.ts | 2 +- .../item-counter/item-counter.component.ts | 3 +- 6 files changed, 79 insertions(+), 22 deletions(-) diff --git a/feature-libs/cart/base/components/cart-shared/cart-item-list/cart-item-list.component.spec.ts b/feature-libs/cart/base/components/cart-shared/cart-item-list/cart-item-list.component.spec.ts index 109604de842..1c262f61f0a 100644 --- a/feature-libs/cart/base/components/cart-shared/cart-item-list/cart-item-list.component.spec.ts +++ b/feature-libs/cart/base/components/cart-shared/cart-item-list/cart-item-list.component.spec.ts @@ -11,9 +11,13 @@ import { PromotionLocation, SelectiveCartFacade, } from '@spartacus/cart/base/root'; -import { I18nTestingModule, UserIdService } from '@spartacus/core'; +import { + FeatureConfigService, + I18nTestingModule, + UserIdService, +} from '@spartacus/core'; import { OutletContextData, PromotionsModule } from '@spartacus/storefront'; -import { Observable, of } from 'rxjs'; +import { Observable, Subject, of } from 'rxjs'; import { CartItemListComponent } from './cart-item-list.component'; class MockActiveCartService { @@ -107,6 +111,12 @@ const mockContext = { }; const context$ = of(mockContext); +class MockFeatureConfigService { + isEnabled() { + return true; + } +} + describe('CartItemListComponent', () => { let component: CartItemListComponent; let fixture: ComponentFixture; @@ -132,6 +142,7 @@ describe('CartItemListComponent', () => { { provide: SelectiveCartFacade, useValue: mockSelectiveCartService }, { provide: MultiCartFacade, useClass: MockMultiCartService }, { provide: UserIdService, useClass: MockUserIdService }, + { provide: FeatureConfigService, useClass: MockFeatureConfigService }, ], }); } @@ -397,6 +408,9 @@ describe('CartItemListComponent', () => { describe('Use outlet with outlet context data', () => { it('should be able to get inputs from outlet context data', () => { + const newContext = structuredClone(mockContext); + newContext.items = [mockItem0]; + const context$ = new Subject(); configureTestingModule().overrideProvider(OutletContextData, { useValue: { context$ }, }); @@ -406,18 +420,16 @@ describe('CartItemListComponent', () => { spyOn(component, '_setItems').and.callThrough(); const setLoading = spyOnProperty(component, 'setLoading', 'set'); component.ngOnInit(); - - expect(component.cartId).toEqual(mockContext.cartId); - expect(component.hasHeader).toEqual(mockContext.hasHeader); - expect(component['_setItems']).toHaveBeenCalledWith(mockContext.items, { - forceRerender: false, + context$.next(newContext); + expect(component.cartId).toEqual(newContext.cartId); + expect(component.hasHeader).toEqual(newContext.hasHeader); + expect(component['_setItems']).toHaveBeenCalledWith(newContext.items, { + forceRerender: true, }); - expect(component.options).toEqual(mockContext.options); - expect(component.promotionLocation).toEqual( - mockContext.promotionLocation - ); - expect(component.readonly).toEqual(mockContext.readonly); - expect(setLoading).toHaveBeenCalledWith(mockContext.cartIsLoading); + expect(component.options).toEqual(newContext.options); + expect(component.promotionLocation).toEqual(newContext.promotionLocation); + expect(component.readonly).toEqual(newContext.readonly); + expect(setLoading).toHaveBeenCalledWith(newContext.cartIsLoading); }); it('should mark view for check and force re-creation of item controls when outlet context emits with changed read-only flag', () => { @@ -443,5 +455,19 @@ describe('CartItemListComponent', () => { component.form.get(mockItem1.entryNumber.toString()) ); }); + + it('should not call _setItems when neither contextRequiresRerender nor isItemsChanged are true', () => { + configureTestingModule().overrideProvider(OutletContextData, { + useValue: { context$ }, + }); + TestBed.compileComponents(); + stubSeviceAndCreateComponent(); + + spyOn(component, '_setItems').and.callThrough(); + component.ngOnInit(); + + // context$ emits mockContext which has the same items that were set in stubSeviceAndCreateComponent + expect(component['_setItems']).not.toHaveBeenCalled(); + }); }); }); diff --git a/feature-libs/cart/base/components/cart-shared/cart-item-list/cart-item-list.component.ts b/feature-libs/cart/base/components/cart-shared/cart-item-list/cart-item-list.component.ts index cfde3524481..521daea91fb 100644 --- a/feature-libs/cart/base/components/cart-shared/cart-item-list/cart-item-list.component.ts +++ b/feature-libs/cart/base/components/cart-shared/cart-item-list/cart-item-list.component.ts @@ -12,6 +12,7 @@ import { OnDestroy, OnInit, Optional, + inject, } from '@angular/core'; import { UntypedFormControl, UntypedFormGroup } from '@angular/forms'; import { @@ -24,7 +25,7 @@ import { SelectiveCartFacade, CartOutlets, } from '@spartacus/cart/base/root'; -import { UserIdService } from '@spartacus/core'; +import { FeatureConfigService, UserIdService } from '@spartacus/core'; import { OutletContextData } from '@spartacus/storefront'; import { Observable, Subscription } from 'rxjs'; import { map, startWith, tap } from 'rxjs/operators'; @@ -84,6 +85,7 @@ export class CartItemListComponent implements OnInit, OnDestroy { } } readonly CartOutlets = CartOutlets; + private featureConfigService = inject(FeatureConfigService); constructor( protected activeCartService: ActiveCartFacade, protected selectiveCartService: SelectiveCartFacade, @@ -133,15 +135,35 @@ export class CartItemListComponent implements OnInit, OnDestroy { if (context.cartIsLoading !== undefined) { this.setLoading = context.cartIsLoading; } - if (context.items !== undefined) { - this.cd.markForCheck(); - this._setItems(context.items, { - forceRerender: contextRequiresRerender, - }); - } + this.updateItemsOnContextChange(context, contextRequiresRerender); }); } + protected updateItemsOnContextChange( + context: ItemListContext, + contextRequiresRerender: boolean + ) { + const preventRedundantRecreationEnabled = + this.featureConfigService.isEnabled( + 'a11yPreventCartItemsFormRedundantRecreation' + ); + if ( + context.items !== undefined && + (!preventRedundantRecreationEnabled || + contextRequiresRerender || + this.isItemsChanged(context.items)) + ) { + this.cd.markForCheck(); + this._setItems(context.items, { + forceRerender: contextRequiresRerender, + }); + } + } + + protected isItemsChanged(newItems: OrderEntry[]): boolean { + return JSON.stringify(this.items) !== JSON.stringify(newItems); + } + /** * Resolves items passed to component input and updates 'items' field */ diff --git a/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts b/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts index b9b08d7dcff..00e9b5395e7 100644 --- a/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts +++ b/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts @@ -275,6 +275,12 @@ export interface FeatureTogglesInterface { */ a11ySemanticPaginationLabel?: boolean; + /** + * When using CartItemListComponent as an outlet ([cxOutlet]="CartOutlets.CART_ITEM_LIST"): + * prevents the form from being recreated when neither the items nor other dependent properties (e.g., readonly) have changed. + */ + a11yPreventCartItemsFormRedundantRecreation?: boolean; + /** * Prevents screen reader from stopping on invisible elements when being in read mode for `BreadcrumbComponent`, `QuickOrderFormComponent` */ @@ -399,6 +405,7 @@ export const defaultFeatureToggles: Required = { a11yVisibleFocusOverflows: false, a11yTruncatedTextForResponsiveView: false, a11ySemanticPaginationLabel: false, + a11yPreventCartItemsFormRedundantRecreation: false, a11yPreventSRFocusOnHiddenElements: false, a11yMyAccountLinkOutline: false, a11yCloseProductImageBtnFocus: false, diff --git a/projects/storefrontapp/src/app/spartacus/spartacus-features.module.ts b/projects/storefrontapp/src/app/spartacus/spartacus-features.module.ts index a661776fe71..3cf0048a3e6 100644 --- a/projects/storefrontapp/src/app/spartacus/spartacus-features.module.ts +++ b/projects/storefrontapp/src/app/spartacus/spartacus-features.module.ts @@ -314,6 +314,7 @@ if (environment.estimatedDeliveryDate) { a11yVisibleFocusOverflows: true, a11yTruncatedTextForResponsiveView: true, a11ySemanticPaginationLabel: true, + a11yPreventCartItemsFormRedundantRecreation: true, a11yMyAccountLinkOutline: true, a11yCloseProductImageBtnFocus: true, a11yNotificationPreferenceFieldset: true, diff --git a/projects/storefrontlib/shared/components/item-counter/item-counter.component.spec.ts b/projects/storefrontlib/shared/components/item-counter/item-counter.component.spec.ts index 61d62c2762d..ef1b5d87c3e 100644 --- a/projects/storefrontlib/shared/components/item-counter/item-counter.component.spec.ts +++ b/projects/storefrontlib/shared/components/item-counter/item-counter.component.spec.ts @@ -227,7 +227,7 @@ describe('ItemCounterComponent', () => { input.dispatchEvent(new KeyboardEvent('keyup', { key: 'Enter' })); fixture.detectChanges(); - expect(component.control.value).toEqual('10'); + expect(component.control.value).toEqual(10); }); }); }); diff --git a/projects/storefrontlib/shared/components/item-counter/item-counter.component.ts b/projects/storefrontlib/shared/components/item-counter/item-counter.component.ts index 6675aa46056..7b764980869 100644 --- a/projects/storefrontlib/shared/components/item-counter/item-counter.component.ts +++ b/projects/storefrontlib/shared/components/item-counter/item-counter.component.ts @@ -113,7 +113,8 @@ export class ItemCounterComponent implements OnInit, OnDestroy { * It is used to improve keyboard controls of the component. */ updateValue(): void { - this.control.setValue(this.input.nativeElement.value); + // Convert string value to number(prevents us from having '1' + 1 = '11' on increment) + this.control.setValue(+this.input.nativeElement.value); this.control.markAsDirty(); }