Ionic-framework: bug: merging custom slides options does not work with nested objects such as pagination

[sebastian-zarzycki-es]

Bug Report

Ionic version:
[ ] 4.x
[x] 5.x

Current behavior:
Options passed via "options" property are not merged with the default options object, they are assigned instead.

Expected behavior:
Object.assign is not merge. They should be merged, as in: properties not specified in new object should not override old ones.

Steps to reproduce:
Try to pass options related to pagination in ion-slides, i.e.:
<ion-slides pager="true" [options]="{ pagination: { clickable: true} }">
This will override whole pagination object beneath and break pagination altogether.

Related code:

var options = { pagination: {a: true, b: false, c: 1, d: '2'}}; 
var customOptions = { pagination: {a: false}}; 
var result = Object.assign(options, customOptions);

result is equal to customOptions, but should be { pagination: {a: false, b: false, c: 1, d: '2'}};

Other information:
None

Ionic info:

Ionic:

   Ionic CLI                     : 6.11.6 (/opt/local/lib/node_modules/@ionic/cli)
   Ionic Framework               : @ionic/angular 5.3.2
   @angular-devkit/build-angular : 0.1001.0
   @angular-devkit/schematics    : 10.0.6
   @angular/cli                  : 10.1.0
   @ionic/angular-toolkit        : 2.3.1

Cordova:

   Cordova CLI       : 10.0.0
   Cordova Platforms : not available
   Cordova Plugins   : not available

Utility:

   cordova-res (update available: 0.15.1) : 0.14.0
   native-run (update available: 1.0.0)   : 0.3.0

System:

   Android SDK Tools : 26.1.1 (/Users/rattkin/Library/Android/sdk)
   ios-deploy        : 1.10.0
   ios-sim           : 8.0.2
   NodeJS            : v12.18.3 (/opt/local/bin/node)
   npm               : 6.14.8
   OS                : macOS Catalina
   Xcode             : Xcode 11.7 Build version 11E801a

[indraraj26]

It is not related to ionic framework
Explanation: https://stackoverflow.com/a/34986585/10842900
Solution would be

var options = { pagination: {a: true, b: false, c: 1, d: '2'}}; 
var customOptions = { pagination: {a: false}}; 
var result = {}
result.pagination = Object.assign(options.pagination, customOptions.pagination);
console.log(result)

[sebastian-zarzycki-es]

It is related to Ionic framework because it uses Object.assign on whole options object (in fact, three times, merging with couple of other objects), rendering options property almost unusable. Shallow merge shouldn't be used here because it results in mangling the default options object. Lodash's _.merge could be used, for all I care, as long as it works properly.

[liamdebeasi]

Thanks for the issue, could you provide a code reproduction via a GitHub repo?

[ionitron-bot[bot]]

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please provide a reproduction with the minimum amount of code required to reproduce the issue. Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

For a guide on how to create a good reproduction, see our Contributing Guide.

[sebastian-zarzycki-es]

Not really. But simple <ion-slides pager="true" [options]="{ pagination: { clickable: true }}"> should demonstate the problem well. Attempting to override just one property results in overriding whole pagination object, rendering slides paginator broken.

[liamdebeasi]

Object.assign does not merge nested objects, so we likely need to do some additional work to handle nested param objects such as pagination.

As a workaround, you can do the following:

import { ViewChild } from '@angular/core';
import { IonSlides } from '@ionic/angular';

...

@ViewChild('slides', { static: true }) slides: IonSlides;

...

async ngOnInit() {
  const swiper = await this.slides.getSwiper();

  swiper.params.pagination.clickable = true;
  swiper.pagination.init();
}

<ion-slides #slides [pager]="true">
  <ion-slide>Slide 1</ion-slide>
  <ion-slide>Slide 2</ion-slide>
  <ion-slide>Slide 3</ion-slide>
</ion-slides>

[sebastian-zarzycki-es]

Yep, thanks. I've figured that one, too. Still, the property field should be handled properly (or removed and users should be redirected to the way of doing this above).