From 5ffa2cce413f1f89b0caa8a8791aa3aa9e6040ea Mon Sep 17 00:00:00 2001 From: Danilo Hoffmann Date: Mon, 29 Apr 2024 10:38:32 +0200 Subject: [PATCH] refactor: disconnect variations from product view model (#1644) BREAKING CHANGE: The variations property on the product view interface was removed. Variations can now be retrieved via the product context facade or the shopping facade. --- docs/guides/migrations.md | 6 +++ .../core/facades/product-context.facade.ts | 22 ++++++++++- src/app/core/facades/shopping.facade.ts | 8 ++++ .../product-variation.helper.spec.ts | 38 ++++++++++--------- .../product-variation.helper.ts | 32 ++++++++++------ .../models/product-view/product-view.model.ts | 6 --- .../products/products.selectors.spec.ts | 15 +++++--- .../shopping/products/products.selectors.ts | 14 +++---- ...roduct-master-variations.component.spec.ts | 6 +-- .../product-master-variations.component.ts | 9 +++-- ...product-variation-select.component.spec.ts | 2 +- .../product-variation-select.component.ts | 8 ++-- 12 files changed, 104 insertions(+), 62 deletions(-) diff --git a/docs/guides/migrations.md b/docs/guides/migrations.md index d07d453231..42c03c40e1 100644 --- a/docs/guides/migrations.md +++ b/docs/guides/migrations.md @@ -39,6 +39,12 @@ The dead code detection script was improved and extended and is now checking Ang This resulted in more variables and methods being declared as `private` and some unused code being removed. This should not affect PWA based projects as long as such internal declarations were not used, else compiling will fail and the code would need to be adapted/reverted accordingly. +Product variations were eagerly loaded via effects. +In projects with a lot of Variations, this can lead to performance issues, especially if the variations data is not needed for the current views. +For that reason product variations are now loaded lazily through the following changes that might need adaptions with project customizations. + +- The variations property on the product view interface was removed. Variations can now be retrieved via the product context facade or the shopping facade. + ## From 5.0 to 5.1 The OrderListComponent is strictly presentational, components using it have to supply the data. diff --git a/src/app/core/facades/product-context.facade.ts b/src/app/core/facades/product-context.facade.ts index 370dbe79e7..4fb51af9ee 100644 --- a/src/app/core/facades/product-context.facade.ts +++ b/src/app/core/facades/product-context.facade.ts @@ -23,7 +23,12 @@ import { Pricing } from 'ish-core/models/price/price.model'; import { ProductLinksDictionary } from 'ish-core/models/product-links/product-links.model'; import { ProductVariationHelper } from 'ish-core/models/product-variation/product-variation.helper'; import { ProductView } from 'ish-core/models/product-view/product-view.model'; -import { ProductCompletenessLevel, ProductHelper, SkuQuantityType } from 'ish-core/models/product/product.model'; +import { + ProductCompletenessLevel, + ProductHelper, + SkuQuantityType, + VariationProduct, +} from 'ish-core/models/product/product.model'; import { Promotion } from 'ish-core/models/promotion/promotion.model'; import { generateProductUrl } from 'ish-core/routing/product/product.route'; import { mapToProperty, whenTruthy } from 'ish-core/utils/operators'; @@ -102,6 +107,7 @@ export interface ProductContext { links: ProductLinksDictionary; promotions: Promotion[]; parts: SkuQuantityType[]; + variations: VariationProduct[]; // variation handling variationCount: number; @@ -138,6 +144,13 @@ export class ProductContextFacade extends RxState implements OnD mapToProperty('sku') ); + private masterProductSKU$ = this.select('product').pipe( + switchMap(product => + ProductHelper.isMasterProduct(product) ? this.validProductSKU$ : this.select('product', 'productMasterSKU') + ), + whenTruthy() + ); + constructor( private shoppingFacade: ShoppingFacade, private appFacade: AppFacade, @@ -441,6 +454,9 @@ export class ProductContextFacade extends RxState implements OnD }; switch (k1) { + case 'variations': + wrap('variations', this.shoppingFacade.productVariations$(this.masterProductSKU$)); + break; case 'links': wrap('links', this.shoppingFacade.productLinks$(this.validProductSKU$)); break; @@ -499,7 +515,9 @@ export class ProductContextFacade extends RxState implements OnD } changeVariationOption(name: string, value: string) { - this.set('sku', () => ProductVariationHelper.findPossibleVariation(name, value, this.get('product'))); + this.set('sku', () => + ProductVariationHelper.findPossibleVariation(name, value, this.get('product'), this.get('variations')) + ); } addToBasket() { diff --git a/src/app/core/facades/shopping.facade.ts b/src/app/core/facades/shopping.facade.ts index 0722a5613b..242930a5fc 100644 --- a/src/app/core/facades/shopping.facade.ts +++ b/src/app/core/facades/shopping.facade.ts @@ -36,6 +36,7 @@ import { getProductLinks, getProductParts, getProductVariationCount, + getProductVariations, loadProduct, loadProductIfNotLoaded, loadProductLinks, @@ -152,6 +153,13 @@ export class ShoppingFacade { ); } + productVariations$(sku: string | Observable) { + return toObservable(sku).pipe( + whenTruthy(), + switchMap(plainSKU => this.store.pipe(select(getProductVariations(plainSKU)))) + ); + } + productVariationCount$(sku: string) { return toObservable(sku).pipe(switchMap(plainSKU => this.store.pipe(select(getProductVariationCount(plainSKU))))); } diff --git a/src/app/core/models/product-variation/product-variation.helper.spec.ts b/src/app/core/models/product-variation/product-variation.helper.spec.ts index 628312cdde..0496e75e8e 100644 --- a/src/app/core/models/product-variation/product-variation.helper.spec.ts +++ b/src/app/core/models/product-variation/product-variation.helper.spec.ts @@ -67,21 +67,15 @@ const productMaster = { ], } as VariationProductMaster; -const variationProductView = { +const variationProduct = { ...productVariations[0], productMaster, - variations: productVariations, -} as ProductView; - -const masterProductView = { - ...productMaster, - variations: productVariations, } as ProductView; describe('Product Variation Helper', () => { describe('buildVariationOptionGroups', () => { it('should build variation option groups for variation product', () => { - const result = ProductVariationHelper.buildVariationOptionGroups(variationProductView); + const result = ProductVariationHelper.buildVariationOptionGroups(variationProduct, productVariations); expect(result).toMatchInlineSnapshot(` [ { @@ -176,19 +170,27 @@ describe('Product Variation Helper', () => { describe('findPossibleVariation', () => { it('should find perfect match when first attribute is changed', () => { - expect(ProductVariationHelper.findPossibleVariation('a2', 'B', variationProductView)).toEqual('333'); + expect(ProductVariationHelper.findPossibleVariation('a2', 'B', variationProduct, productVariations)).toEqual( + '333' + ); }); it('should find perfect match when second attribute is changed', () => { - expect(ProductVariationHelper.findPossibleVariation('a1', 'B', variationProductView)).toEqual('444'); + expect(ProductVariationHelper.findPossibleVariation('a1', 'B', variationProduct, productVariations)).toEqual( + '444' + ); }); it('should find variation match when second attribute is changed and no perfect match could be found', () => { - expect(ProductVariationHelper.findPossibleVariation('a2', 'C', variationProductView)).toEqual('666'); + expect(ProductVariationHelper.findPossibleVariation('a2', 'C', variationProduct, productVariations)).toEqual( + '666' + ); }); it('should return original sku when impossible selection is selected', () => { - expect(ProductVariationHelper.findPossibleVariation('a2', 'Z', variationProductView)).toEqual('222'); + expect(ProductVariationHelper.findPossibleVariation('a2', 'Z', variationProduct, productVariations)).toEqual( + '222' + ); }); }); @@ -198,7 +200,7 @@ describe('Product Variation Helper', () => { }); it('should use variation length when no filters are given', () => { - expect(ProductVariationHelper.productVariationCount(masterProductView, undefined)).toEqual(5); + expect(ProductVariationHelper.productVariationCount(productVariations, undefined)).toEqual(5); }); it('should ignore irrelevant selections when counting', () => { @@ -211,7 +213,7 @@ describe('Product Variation Helper', () => { ], } as FilterNavigation; - expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(5); + expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(5); }); it('should filter for products matching single selected attributes', () => { @@ -231,7 +233,7 @@ describe('Product Variation Helper', () => { ], } as FilterNavigation; - expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(2); + expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(2); }); it('should filter for products matching complex value attributes', () => { @@ -244,7 +246,7 @@ describe('Product Variation Helper', () => { ], } as FilterNavigation; - expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(3); + expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(3); }); it('should filter for products matching multiple selected attributes', () => { @@ -260,7 +262,7 @@ describe('Product Variation Helper', () => { ], } as FilterNavigation; - expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(3); + expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(3); }); it('should filter for products matching multiple selected attributes over multiple facets', () => { @@ -277,7 +279,7 @@ describe('Product Variation Helper', () => { ], } as FilterNavigation; - expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(1); + expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(1); }); }); }); diff --git a/src/app/core/models/product-variation/product-variation.helper.ts b/src/app/core/models/product-variation/product-variation.helper.ts index e0ccab4743..50b8bbb067 100644 --- a/src/app/core/models/product-variation/product-variation.helper.ts +++ b/src/app/core/models/product-variation/product-variation.helper.ts @@ -2,6 +2,7 @@ import { groupBy } from 'lodash-es'; import { FilterNavigation } from 'ish-core/models/filter-navigation/filter-navigation.model'; import { ProductView } from 'ish-core/models/product-view/product-view.model'; +import { VariationProduct } from 'ish-core/models/product/product.model'; import { omit } from 'ish-core/utils/functions'; import { VariationAttribute } from './variation-attribute.model'; @@ -12,7 +13,7 @@ export class ProductVariationHelper { /** * Build select value structure */ - static buildVariationOptionGroups(product: ProductView): VariationOptionGroup[] { + static buildVariationOptionGroups(product: ProductView, variations: VariationProduct[]): VariationOptionGroup[] { if (!product?.variableVariationAttributes?.length) { return []; } @@ -38,7 +39,7 @@ export class ProductVariationHelper { })) .map(option => ({ ...option, - alternativeCombination: ProductVariationHelper.alternativeCombinationCheck(option, product), + alternativeCombination: ProductVariationHelper.alternativeCombinationCheck(option, product, variations), })); // group options list by attributeId @@ -57,13 +58,18 @@ export class ProductVariationHelper { }); } - static findPossibleVariation(name: string, value: string, product: ProductView): string { + static findPossibleVariation( + name: string, + value: string, + product: ProductView, + variations: VariationProduct[] + ): string { const target = omit( ProductVariationHelper.simplifyVariableVariationAttributes(product.variableVariationAttributes), name ); - const candidates = product.variations + const candidates = variations .filter(variation => variation.variableVariationAttributes.some( attr => attr.variationAttributeId === name && ProductVariationHelper.isEqual(attr.value, value) @@ -92,11 +98,11 @@ export class ProductVariationHelper { return product && !!product.defaultVariationSKU; } - static productVariationCount(product: ProductView, filters: FilterNavigation): number { - if (!product) { + static productVariationCount(variations: VariationProduct[], filters: FilterNavigation): number { + if (!variations?.length) { return 0; } else if (!filters?.filter) { - return product.variations?.length; + return variations?.length; } const selectedFacets = filters.filter @@ -105,10 +111,10 @@ export class ProductVariationHelper { .map(selected => selected.split('=')); if (!selectedFacets.length) { - return product.variations?.length; + return variations?.length; } - return product.variations + return variations .map(p => p.variableVariationAttributes) .filter(attrs => attrs.every( @@ -130,7 +136,11 @@ export class ProductVariationHelper { * @param product The given product containing the related product variations * @returns Indicates if no perfect match is found. */ - private static alternativeCombinationCheck(option: VariationSelectOption, product: ProductView): boolean { + private static alternativeCombinationCheck( + option: VariationSelectOption, + product: ProductView, + variations: VariationProduct[] + ): boolean { if (!product.variableVariationAttributes?.length) { return; } @@ -148,7 +158,7 @@ export class ProductVariationHelper { } // check if one of the variation products has the same attributes as the attributes to be compared - return !product.variations.some(variation => + return !variations.some(variation => ProductVariationHelper.variationAttributeArrayEquals(variation.variableVariationAttributes, comparisonAttributes) ); } diff --git a/src/app/core/models/product-view/product-view.model.ts b/src/app/core/models/product-view/product-view.model.ts index 7565695a13..5a26025e51 100644 --- a/src/app/core/models/product-view/product-view.model.ts +++ b/src/app/core/models/product-view/product-view.model.ts @@ -11,13 +11,11 @@ interface SimpleProductView extends Product { interface VariationProductView extends VariationProduct, SimpleProductView { type: VariationProduct['type']; - variations: VariationProduct[]; productMaster: VariationProductMaster; } interface VariationProductMasterView extends VariationProductMaster, SimpleProductView { type: VariationProductMaster['type']; - variations: VariationProduct[]; defaultVariationSKU: string; } @@ -33,7 +31,6 @@ export function createProductView(product: Product, defaultCategory?: CategoryVi export function createVariationProductMasterView( product: VariationProductMaster, defaultVariationSKU: string, - variations: VariationProduct[], defaultCategory?: CategoryView ): VariationProductMasterView { return ( @@ -41,14 +38,12 @@ export function createVariationProductMasterView( ...createProductView(product, defaultCategory), type: 'VariationProductMaster', defaultVariationSKU, - variations, } ); } export function createVariationProductView( product: VariationProduct, - variations: VariationProduct[], productMaster: VariationProductMaster, defaultCategory?: CategoryView ): VariationProductView { @@ -56,7 +51,6 @@ export function createVariationProductView( product && { ...createProductView(product, defaultCategory), type: 'VariationProduct', - variations, productMaster, } ); diff --git a/src/app/core/store/shopping/products/products.selectors.spec.ts b/src/app/core/store/shopping/products/products.selectors.spec.ts index 3143e4a8f4..853e49515b 100644 --- a/src/app/core/store/shopping/products/products.selectors.spec.ts +++ b/src/app/core/store/shopping/products/products.selectors.spec.ts @@ -27,6 +27,7 @@ import { getProductEntities, getProductLinks, getProductParts, + getProductVariations, getSelectedProduct, } from './products.selectors'; @@ -330,14 +331,16 @@ describe('Products Selectors', () => { "defaultVariationSKU": "VAR", "sku": "SKU", "type": "VariationProductMaster", - "variations": [ - { - "sku": "VAR", - "type": "VariationProduct", - }, - ], } `); + expect(getProductVariations('SKU')(store$.state)).toMatchInlineSnapshot(` + [ + { + "sku": "VAR", + "type": "VariationProduct", + }, + ] + `); }); }); diff --git a/src/app/core/store/shopping/products/products.selectors.ts b/src/app/core/store/shopping/products/products.selectors.ts index 2c5149f177..ed2dfa52d0 100644 --- a/src/app/core/store/shopping/products/products.selectors.ts +++ b/src/app/core/store/shopping/products/products.selectors.ts @@ -64,7 +64,7 @@ export const getProductVariationSKUs = (sku: string) => return state.variations[sku]; }); -const internalProductVariations = (sku: string) => +export const getProductVariations = (sku: string) => /* memoization manually by output: variation SKUs don't vary, but reference to state changes often */ createSelectorFactory(projector => resultMemoize(projector, isEqual))( getProductsState, @@ -94,13 +94,12 @@ export const getProduct = (sku: string) => internalRawProduct(sku), internalProductDefaultCategory(sku), internalProductDefaultVariationSKU(sku), - internalProductVariations(sku), internalProductMaster(sku), - (product, defaultCategory, defaultVariationSKU, variations, productMaster): ProductView => + (product, defaultCategory, defaultVariationSKU, productMaster): ProductView => ProductHelper.isMasterProduct(product) - ? createVariationProductMasterView(product, defaultVariationSKU, variations, defaultCategory) + ? createVariationProductMasterView(product, defaultVariationSKU, defaultCategory) : ProductHelper.isVariationProduct(product) - ? createVariationProductView(product, variations, productMaster, defaultCategory) + ? createVariationProductView(product, productMaster, defaultCategory) : createProductView(product, defaultCategory) ); @@ -112,8 +111,9 @@ export const getProductVariationCount = (sku: string) => createSelector( getAvailableFilter, getProduct(sku), - (filters, product) => - ProductHelper.isMasterProduct(product) && ProductVariationHelper.productVariationCount(product, filters) + getProductVariations(sku), + (filters, product, variations) => + ProductHelper.isMasterProduct(product) && ProductVariationHelper.productVariationCount(variations, filters) ); export const getProductLinks = (sku: string) => createSelector(getProductsState, state => state.links[sku]); diff --git a/src/app/pages/product/product-master-variations/product-master-variations.component.spec.ts b/src/app/pages/product/product-master-variations/product-master-variations.component.spec.ts index 4f2da9e73a..ade952d461 100644 --- a/src/app/pages/product/product-master-variations/product-master-variations.component.spec.ts +++ b/src/app/pages/product/product-master-variations/product-master-variations.component.spec.ts @@ -6,6 +6,7 @@ import { instance, mock, when } from 'ts-mockito'; import { ProductContextFacade } from 'ish-core/facades/product-context.facade'; import { ProductView } from 'ish-core/models/product-view/product-view.model'; +import { VariationProduct } from 'ish-core/models/product/product.model'; import { FilterNavigationComponent } from 'ish-shared/components/filter/filter-navigation/filter-navigation.component'; import { ProductListingComponent } from 'ish-shared/components/product/product-listing/product-listing.component'; @@ -19,9 +20,8 @@ describe('Product Master Variations Component', () => { beforeEach(async () => { const context = mock(ProductContextFacade); when(context.select('product', 'sku')).thenReturn(of('123456789')); - when(context.select('product')).thenReturn( - of({ type: 'VariationProductMaster', variations: [{ sku: '123456789-01' }] } as ProductView) - ); + when(context.select('product')).thenReturn(of({ type: 'VariationProductMaster' } as ProductView)); + when(context.select('variations')).thenReturn(of([{ sku: '123456789-01' }] as VariationProduct[])); await TestBed.configureTestingModule({ declarations: [ diff --git a/src/app/pages/product/product-master-variations/product-master-variations.component.ts b/src/app/pages/product/product-master-variations/product-master-variations.component.ts index b411d26047..ccd05584fb 100644 --- a/src/app/pages/product/product-master-variations/product-master-variations.component.ts +++ b/src/app/pages/product/product-master-variations/product-master-variations.component.ts @@ -1,7 +1,7 @@ import { ViewportScroller } from '@angular/common'; import { ChangeDetectionStrategy, Component, OnInit } from '@angular/core'; import { ActivationStart, NavigationEnd, NavigationStart, Router } from '@angular/router'; -import { Observable } from 'rxjs'; +import { Observable, combineLatest } from 'rxjs'; import { debounce, filter, map, takeUntil } from 'rxjs/operators'; import { ProductContextFacade } from 'ish-core/facades/product-context.facade'; @@ -23,9 +23,10 @@ export class ProductMasterVariationsComponent implements OnInit { ngOnInit() { this.sku$ = this.context.select('product', 'sku'); this.categoryId$ = this.context.select('categoryId'); - this.hasVariations$ = this.context - .select('product') - .pipe(map(product => ProductHelper.isMasterProduct(product) && product.variations?.length > 0)); + this.hasVariations$ = combineLatest([ + this.context.select('product').pipe(map(product => ProductHelper.isMasterProduct(product))), + this.context.select('variations').pipe(map(variations => variations?.length > 0)), + ]).pipe(map(([isMaster, hasVariations]) => isMaster && hasVariations)); this.router.events .pipe( diff --git a/src/app/shared/components/product/product-variation-select/product-variation-select.component.spec.ts b/src/app/shared/components/product/product-variation-select/product-variation-select.component.spec.ts index 639729ee29..bbeed085bb 100644 --- a/src/app/shared/components/product/product-variation-select/product-variation-select.component.spec.ts +++ b/src/app/shared/components/product/product-variation-select/product-variation-select.component.spec.ts @@ -46,7 +46,6 @@ describe('Product Variation Select Component', () => { const variationProductView = { ...variationProduct, - variations: [variationProduct], productMaster, } as ProductView; @@ -69,6 +68,7 @@ describe('Product Variation Select Component', () => { element = fixture.nativeElement; when(context.select('product')).thenReturn(of(variationProductView)); + when(context.select('variations')).thenReturn(of([variationProduct])); when(context.select('displayProperties', 'variations')).thenReturn(of(true)); }); diff --git a/src/app/shared/components/product/product-variation-select/product-variation-select.component.ts b/src/app/shared/components/product/product-variation-select/product-variation-select.component.ts index 6714e7f261..c3a296fe13 100644 --- a/src/app/shared/components/product/product-variation-select/product-variation-select.component.ts +++ b/src/app/shared/components/product/product-variation-select/product-variation-select.component.ts @@ -1,5 +1,5 @@ import { ChangeDetectionStrategy, Component, OnInit } from '@angular/core'; -import { Observable } from 'rxjs'; +import { Observable, combineLatest } from 'rxjs'; import { map } from 'rxjs/operators'; import { v4 as uuid } from 'uuid'; @@ -20,9 +20,9 @@ export class ProductVariationSelectComponent implements OnInit { constructor(private context: ProductContextFacade) {} ngOnInit() { - this.variationOptions$ = this.context - .select('product') - .pipe(map(ProductVariationHelper.buildVariationOptionGroups)); + this.variationOptions$ = combineLatest([this.context.select('product'), this.context.select('variations')]).pipe( + map(([product, variations]) => ProductVariationHelper.buildVariationOptionGroups(product, variations)) + ); this.visible$ = this.context.select('displayProperties', 'variations'); }