Retrofit rather than mixin parameters

[arthur-clifford]

Hi guys,

I saw recent activity regarding column grouping etc. It reminded me of some of what I had to go through in getting my stuff working and I wanted to share an idea that I think would help some on the column problem but is a little more system wide.

While it has been a while since I looked at most of this stuff. One thing I remember thinking when I was, that rather than using a mixin function for merging parameters into the various slick grid classes, you might want to pass around a reference to the original parameters object.

Excuse me if I coders-plain a moment to make sure we are on the same page:

As OOP coders we know that because of passing by reference the following is true:

class SomeClass {
   public x = 1
}

var a = new SomeClass() 
b = a;
b.x = 2;
console.log('value of x for a = '+a.x)
// logs out
value of x for a = 2

The reason this is true, is because b and a reference the same memory space. b = a is passing (or assigning) by reference.

In JavaScript (and typescript) all objects are passed by reference.

The slickgrid tech used a clever technique of defining a parameters object in each class and when parameters are passed in, a mixin function is used to mix the user specified parameters into the class parameters. This means if the user specified parameters are set they override the default parameters. A smart way to setup defaults and override them.

However, the result is that it takes a reference to params and copies its values locally into the current object/function.

At that moment, the relationship between the user defined parameters object and the internal slickgrid code is broken and to only to programmatically know what slickgrid is using for the parameters is to hope there is a function for getting information back out

An alternate approach:
Consider a retrofit method

Retrofit(targetObj:any, srcObj:any) {
    for(const key in srcObj) {
         if(srcObj.hasOwnProperty(key)) {
              if(!targetObj.hasOwnProperty(key)) {
                   targetObj[key] = srcObj[key];
              }
         }
     }
}

Note: this would only set parameters that aren't already set. It is an inverted variation of the mixin approach but accomplishes the same thing in terms of having an in ternal params object that is guranteed to have defaults while also having user overrides. It has the added benefit of reflecting the current state of the original parameters object.

Now consider:

var params = {
    gridTitle:'cool grid'
}

init(params) {
   var parameters = params;
   paramDefaults = {param1:'some val'};
  Retrofit(parameters , paramDefaults);
}

init(params);
console.log(params.param1)
// logs 'some val'

The retrofit approach means that you are internally using the original params object, which if maintained in the controller code is something the developer (outside of slickgrid) can turn to for the current state of the grid or modifications to the original parameters.

If this approach were entertained, the idea would be to provide a detectChanges function on the main grid object so that you could make changes to the params object after initializing sliickgrid and tell the grid to update. There would be no need to pass the parameters back in because the grid already has a reference to it.

Ghislan I'm not sure if you know about Angular's ChangeDetectionStrategy and ChangeDetectionReference techniques, but this is in the same space as that methodology. The idea being either there would need to be an are-we-there yet polling for changes or a manual trigger for detecting changes. For a component you can set the detectionStrategy to ChangeDetectionStrategy.OnPush and then in your constructor params have private cdr:ChangeDetectionReference which mean when you change something databound in the component you call cdr.detectChanges(). I've started doing that as much as possible as it means handlers get called only when needed.

Given the angular slickgrid component data binds to the parameters object which means its pretty much kept in memory in the app. It would make more sense to use that object as the go-to place for information about what has changed. Except, as things are now, slickgrid doesn't have a reference to that object it breaks it as soon as the mixins happen.

If there are any concerns about what to do if you want to revert to the original parameters, well if that is a concern I can clone the original parameters before passing it to slickgrid and clone it back to the data-bound parameters if I really have to.

With regard to the column tech. Consider the retrofit model where everything is using the same parameters reference decorated with the current column setup. The problems seen in the reordering and tree etc has to do with things getting out of sync which is happening because several things are maintaining their own "copy" of the column info rather than manipulating a common reference.

There may be down sides to this I'm not aware of, but at the moment I think slick-grid would be slicker if it used a retrofit pattern rather than mixin and decorated the parameters object reference with current grid state info that could be accessed by the developer at runtime.

[6pac]

Thanks for your comments @arthur-clifford! I'll have a look at this more closely soon and get back to you. I've noticed this too, and your points are valid. There are various schools of thought around the issue and I'd need to have a think and do a little bit of research. That will take a bit of time.

[ghiscoding]

I'd be happy to see a PR for this change, I could then run all my Cypress E2E tests for all my libs and see if anything breaks.

[6pac]

@arthur-clifford after thinking about it a bit, it seems to me that the key element here is the change detection event. in either scenario, the grid or component needs to know when an option has changed at the other end. we have a onSetOptions event that compnents can use. given the existence of that event, I'm not sure that it really matters which strategy is used (the current one is more work to update, but probably has some advantages)? perhaps components just need to utilise the event better?

Also, you mention the columns, can I confirm that here we are just talking about the options object or are you also talking about the columns array?

[arthur-clifford]

I was thinking about this a little bit and If you have concerns about providing write access to a variable after startup but still are ok with the user having access to information through the params list you could use the Object.createProperty method to define a property with a getter function that gets a copy of the current value and a setter that either does nothing or provides some degree of sanitation or whatever you feel is necessary. Object.defineProperty() - JavaScript | MDN (mozilla.org) <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty> That would useful, for example, decorating the parameters object with a rows parameter that just has a getter that returns a copy of the rows list rather than a reference. When one is playing with custom rows (which was what I was doing) having access to the data only is not sufficient since row and data aren’t necessarily a 1:1 relationship. However, you wouldn’t want people necessarily mucking with creating rows arbitrarily and externally to the grid/view as that would likely create problems. So, having a getter only property would solve that problem.

[6pac]

@arthur-clifford could you be specific about what Slickgrid objects you are wanting to change? Not sure if it's the options or columns, or something else.

I note you are correct about how properties are copied when setting and initialising say the options object - in the init code this occurs, which breaks all links to the passed-in options object:

  options = $.extend({}, defaults, options);

However from that point on, internally the reference to this internal version of the options parameter appears to be maintained (ie. is not replaced when set), and this is the getter:

function getOptions() {
  return options;
}

so not sure why simply getting this object and storing it wouldn't provide long term access to the option properties.

Calling setOptions() does trigger the onSetOptions event, and this event may be triggered also by external code if needed.
Does this then cover everything you are suggesting?

The mixin would be a valid approach, but I am worried it may fall into the area of being a breaking change. I know I've had bugs before from inadvertently changing properties of referenced objects, and at present the grid very explicitly breaks the link to the passed in object.

[ghiscoding]

This should now be implemented by PR #831 and released in v5.0, see SlickGrid 5.0 Announcement & Migration