Change defaultBinding on dataset property in aurelia-slickgrid custom element

[jmzagorski]

I'm submitting a feature request

• Framework Version:
  current

Please tell us about your environment:

• Operating System:
  All

• Browser:
  all

• Language:
  TypeScript X.X

Current behavior:
AureliaSlickgridCustomElement has a defaultBinding of twoWay on the dataset property in https://github.com/ghiscoding/aurelia-slickgrid/blob/master/aurelia-slickgrid/src/aurelia-slickgrid/aurelia-slickgrid.ts#L70

Expected/desired behavior:

• What is the expected behavior?
  Do not specify a default binding, let it "default" to aurelia's default of oneWay

• What is the motivation / use case for changing the behavior?
  I do not think we need the overhead of bindingMode.twoWay for the dataset property. The custom aurelia-slickgrid element will never update the dataset, but only get updates from the view model. Besides the slight overhead, there can be issues as I ran into. If a user is using the default dataset.bind in a view and that dataset is a getter property on the view model, the application will throw this error:

ERROR [app-router] TypeError: Cannot set property newDataset of #<Example4> which has only a getter

This issue is probabaly an edge case, but it does produce an Aurelia error that might be hard to debug for users. The work around is to make use dataset.one-way in the view, which will override the custom elements defaultBinding, but some users might not realize this error is coming from this library.

I can replicate this in example4.ts with the diffs below:

<template>
  <h2>${title}</h2>
  <div class="subtitle" innerhtml.bind="subTitle"></div>

  <aurelia-slickgrid grid-id="grid1" dataview.bind="dataview" grid.bind="gridObj" column-definitions.bind="columnDefinitions"
    grid-options.bind="gridOptions" 
- dataset.bind="dataset" 
+ dataset.bind="newDataset" 
asg-on-grid-state-service-changed.delegate="gridStateChanged($event.detail)">
  </aurelia-slickgrid>
</template>

- import {  autoinject } from 'aurelia-framework';
+ import { computedFrom, autoinject } from 'aurelia-framework';
import { CustomInputFilter } from './custom-inputFilter';
import { Column, FieldType, FilterType, Formatter, Formatters, GridOption, GridStateService } from '../../aurelia-slickgrid';

function randomBetween(min, max) {
  return Math.floor(Math.random() * (max - min + 1) + min);
}
const NB_ITEMS = 500;

@autoinject()
export class Example4 {
  title = 'Example 4: Client Side Sort/Filter';
  subTitle = `
  Sort/Filter on client side only using SlickGrid DataView (<a href="https://github.com/ghiscoding/aurelia-slickgrid/wiki/Sorting" target="_blank">Wiki link</a>)
  <br/>
  <ul class="small">
    <li>Support multi-sort (by default), hold "Shift" key and click on the next column to sort.
    <li>All column types support the following operators: (>, >=, <, <=, <>, !=, =, ==, *)
    <ul>
      <li>Example: >100 ... >=2001-01-01 ... >02/28/17</li>
      <li><b>Note:</b> For filters to work properly (default is string), make sure to provide a FieldType (type is against the dataset, not the Formatter)</li>
    </ul>
    <li>Date Filters</li>
    <ul>
      <li>FieldType of dateUtc/date (from dataset) can use an extra option of "filterSearchType" to let user filter more easily. For example, in the "UTC Date" field below, you can type "&gt;02/28/2017", also when dealing with UTC you have to take the time difference in consideration.</li>
    </ul>
    <li>On String filters, (*) can be used as startsWith (Hello* => matches "Hello Word") ... endsWith (*Doe => matches: "John Doe")</li>
    <li>Custom Filter are now possible, "Description" column below, is a customized InputFilter with different placeholder. See <a href="https://github.com/ghiscoding/aurelia-slickgrid/wiki/Custom-Filter" target="_blank">Wiki - Custom Filter</a>
  </ul>
`;

  columnDefinitions: Column[];
  gridOptions: GridOption;
  dataset: any[];

  constructor(private gridStateService: GridStateService) {
    this.defineGrid();
  }

  attached() {
    // populate the dataset once the grid is ready
    this.getData();
  }

  detached() {
    this.saveCurrentGridState();
  }

  /* Define grid Options and Columns */
  defineGrid() {
    // prepare a multiple-select array to filter with
    const multiSelectFilterArray = [];
    for (let i = 0; i < NB_ITEMS; i++) {
      multiSelectFilterArray.push({ value: i, label: i });
    }

    this.columnDefinitions = [
      {
        id: 'title',
        name: 'Title',
        field: 'title',
        filterable: true,
        sortable: true,
        type: FieldType.string,
        minWidth: 45,
        filter: {
          type: FilterType.compoundInput
        }
      },
      {
        id: 'description', name: 'Description', field: 'description', filterable: true, sortable: true, minWidth: 80,
        type: FieldType.string,
        filter: {
          type: FilterType.custom,
          customFilter: new CustomInputFilter() // create a new instance to make each Filter independent from each other
        }
      },
      {
        id: 'duration', name: 'Duration (days)', field: 'duration', sortable: true, type: FieldType.number, exportCsvForceToKeepAsString: true,
        minWidth: 55,
        filterable: true,
        filter: {
          collection: multiSelectFilterArray,
          type: FilterType.multipleSelect,
          searchTerms: [1, 33, 50], // default selection
          // we could add certain option(s) to the "multiple-select" plugin
          filterOptions: {
            maxHeight: 250,
            width: 175
          }
        }
      },
      {
        id: 'complete', name: '% Complete', field: 'percentComplete', formatter: Formatters.percentCompleteBar, minWidth: 70, type: FieldType.number, sortable: true,
        filterable: true, filter: { type: FilterType.compoundInput }
      },
      {
        id: 'start', name: 'Start', field: 'start', formatter: Formatters.dateIso, sortable: true, minWidth: 75, exportWithFormatter: true,
        type: FieldType.date, filterable: true, filter: { type: FilterType.compoundDate }
      },
      {
        id: 'usDateShort', name: 'US Date Short', field: 'usDateShort', sortable: true, minWidth: 70, width: 70,
        type: FieldType.dateUsShort, filterable: true, filter: { type: FilterType.compoundDate }
      },
      {
        id: 'utcDate', name: 'UTC Date', field: 'utcDate', formatter: Formatters.dateTimeIsoAmPm, sortable: true, minWidth: 115,
        type: FieldType.dateUtc, outputType: FieldType.dateTimeIsoAmPm, filterable: true, filter: { type: FilterType.compoundDate }
      },
      {
        id: 'effort-driven', name: 'Effort Driven', field: 'effortDriven', minWidth: 85, maxWidth: 85, formatter: Formatters.checkmark,
        type: FieldType.boolean,
        sortable: true,
        filterable: true,
        filter: {
          collection: [{ value: '', label: '' }, { value: true, label: 'True' }, { value: false, label: 'False' }],
          type: FilterType.singleSelect,

          // we could add certain option(s) to the "multiple-select" plugin
          filterOptions: {
            autoDropWidth: true
          },
        }
      }
    ];

    this.gridOptions = {
      autoResize: {
        containerId: 'demo-container',
        sidePadding: 15
      },
      enableFiltering: true,

      // use columnDef searchTerms OR use presets as shown below
      presets: {
        filters: [
          { columnId: 'duration', searchTerms: [2, 22, 44] },
          // { columnId: 'complete', searchTerm: '5', operator: '>' },
          { columnId: 'usDateShort', operator: '<', searchTerm: '4/20/25' },
          // { columnId: 'effort-driven', searchTerm: true }
        ],
        sorters: [
          { columnId: 'duration', direction: 'DESC' },
          { columnId: 'complete', direction: 'ASC' }
        ],
      }
    };
  }

+  @computedFrom('dataset')
+  get newDataset() {
+    console.log('got dataset');
+    return !this.dataset ? [] : this.dataset.map(d => ({
+      ...d,
+      // create some random computed property here
+    }));
+  }

  getData() {
    // mock a dataset
    this.dataset = [];
    for (let i = 0; i < NB_ITEMS; i++) {
      const randomDuration = Math.round(Math.random() * 100);
      const randomYear = randomBetween(2000, 2025);
      const randomYearShort = randomBetween(10, 25);
      const randomMonth = randomBetween(1, 12);
      const randomMonthStr = (randomMonth < 10) ? `0${randomMonth}` : randomMonth;
      const randomDay = randomBetween(10, 28);
      const randomPercent = randomBetween(0, 100);
      const randomHour = randomBetween(10, 23);
      const randomTime = randomBetween(10, 59);

      this.dataset[i] = {
        id: i,
        title: 'Task ' + i,
        description: (i % 5) ? 'desc ' + i : null, // also add some random to test NULL field
        duration: randomDuration,
        percentComplete: randomPercent,
        percentCompleteNumber: randomPercent,
        start: new Date(randomYear, randomMonth, randomDay),          // provide a Date format
        usDateShort: `${randomMonth}/${randomDay}/${randomYearShort}`, // provide a date US Short in the dataset
        utcDate: `${randomYear}-${randomMonthStr}-${randomDay}T${randomHour}:${randomTime}:${randomTime}Z`,
        effortDriven: (i % 3 === 0)
      };
    }
  }

  /** Dispatched event of a Grid State Changed event */
  gridStateChanged(gridState) {
    console.log('Client sample, Grid State changed:: ', gridState);
  }

  /** Save current Filters, Sorters in LocaleStorage or DB */
  saveCurrentGridState() {
    console.log('Client sample, current Grid State:: ', this.gridStateService.getCurrentGridState());
  }
}

[ghiscoding]

I'm not sure that you can go with a oneWay binding on dataset, especially Example 11 where it adds item on the fly.

You can give it a try, I might be wrong but I thought twoWay was more appropriate. Like in my current App, I a "Create Item" which will add it to the grid (on first row) once the item is created, just like in Example 11

[jmzagorski]

it should still work. I think the two way means that if you reset the dataset to something else it changes in the view model too. Give it a try and let me know your result when you have a chance, the change would be:

-  @bindable({ defaultBindingMode: bindingMode.twoWay }) dataset: any[];
+  @bindable dataset: any[];

if you do discover a potential issue let me know and i can look at it, or this can just be added to the documentation.

[ghiscoding]

Well actually, the Service is probably doing the work, not the binding. So yeah you're probably right. What benefit will that give us? Performance?

Anyhow, can you make sure that the Example 11 still works after changing that. If that is ok, then I don't mind going ahead. We could also include it in the next upcoming release, if you have time to create PR

[jmzagorski]

there are two benefits i can think of

1. Maybe a performance boost? Probably not noticeable, but Aurelia doesn't have to setup the two way communication on a dataset. This is a guess and not tested
2. I can use a @computedFrom property on my viewmodel without specifying one-way in the view without getting that foreign error. So that is less to remember to do/setup for the user. In my app i dynamically generate urls using the router and use that dataset in my grid. For example, my server dataset is elements and my viewmodel dataset is editUrls in my viewmodel:

  @computedFrom('elements')
  get editUrls() {
    return this.elements.map(e => ({
      ...e,
      editUrl: this.router.generate('component-edit', { id: e.id })
    }));
  }

Example 11 does work, but I will be sure to test the other examples. I will submit a PR if you think this is helpful.

EDIT or I can just update the documentation with a "how to" if someone wants to do this? I'll let you decide.

[ghiscoding]

As long as all examples still works, I'm all good with it and yes I know we might not visually see a boost but it will still be there, less twoWay is always better.

I assume you are using the grid without Backend Service API, right? I mean you are simply passing your dataset to the same property, correct? If so, I assume you will be the first person to notice any issues with them, since I'm mostly using the Backend Service API (GraphQL) on my side (in Angular that is, sadly I most admit).

Updating wikis are always good, we can always refer to them if any questions are brought up and these are all helpful. I often go back and read my own wikis, to know how I use things lol. The grid now has so many features, wikis are the best to keep track and be helpful to users.

EDIT
I now understand your @computedFrom, do we always have to use that or does the previous way of using it still works? I wouldn't want to break anyone's code, at least not before a version 2.x.

[jmzagorski]

i agree about wikis, I will do some thorough testing the best I can before making this change.

[ghiscoding]

ok when did u plan to have a PR for this? I was planning a release by tomorrow evening, should I wait for it or go ahead with a release tomorrow?

By the way, I'm going in vacation starting Saturday morning on the 14th. You'll be on your own after (for 3 weeks that is) :P
I don't mind you do releases though

[jmzagorski]

Go ahead and release, I want to make sure I test this enough since it is a small change that has a big impact.

Enjoy your much deserved vacation! I do not think I will need to release anything new while you are gone. From what I have seen so far today since I am working on the grid, it has everything I need.

[jmzagorski]

Just saw

EDIT
I now understand your @computedFrom, do we always have to use that or does the previous way of using it still works? I wouldn't want to break anyone's code, at least not before a version 2.x.

You do not have to use @computedFrom after this change is in. I just used it in the example so aurelia did not keep retrieving the data every second. If we did not use @computedFrom then aurelia would fallback to its dirty checking and call the getter every second.

[ghiscoding]

Sounds good then

[ghiscoding]

closed by #65