this.grid inside column functions points to last grid created instead of current instance

[schallm]

We are using the renderCell function of columns to create elements that allow users to edit items. Once the edit is complete, we may need to refresh the grid that the data came from. We do this by holding on to this.grid and refreshing.

If you have multiple grids on the screen that are created from the created class, the this.grid is the last grid created from that class. I have a fiddle created here showing the issue. Just click on the button and see that all buttons point to the second grid.

My column definition looks like the following:

{
  label: "Actions",
  renderCell: function(object, value, node, options) {
    var grid = this.grid;
    var button = new Button({
      label: "Click Me",
      onClick: function() {
        alert("hello from grid: " + grid.id);
      }
    });
    button.placeAt(node).startup();
  }
}

I believe this is setup as part of the column configuration here?

Can this be changed to be the correct instance of the grid?

[dylans]

I think the issue is that you have a singleton for the column definition, so the local variable grid is updated to point to the current grid. So you would need to do something to preserve the context. Alternatively (and perhaps better) would be to add the event listener using event delegation for all buttons across all grids external to the column definition.

JSFiddle's access to rawcdn is incredibly slow this morning which is preventing me from quickly providing a better working example, but hopefully this makes sense.

[schallm]

The event click was a simplification to recreate the issue. We are creating other widgets that hold on onto the grid to perform several actions. Your statement of singleton for columns is correct. We create the grid definition once so the column definitions are part of the prototype. Since the _configColumns function updates the prototype's column's grid property, when multiple grids are instantiated from the same type, the grid property of column definition points to the last created grid.

A comment in the code mentions the this.grid within the column definition is for plugins. This works fine with 1 instance of a grid type on the page, but even for plugins, this isn't correct once you have two grids created from the same type.

I'm not sure how to fix this with current code. As you mentioned you would need to preserve the context for each instance of a grid somehow. Maybe have a copy of columns for each instantiated grid?

A potential and easy workaround would be to add the current grid to the options passed around during rendering. Since this code originates inside of List.js, my addition adds a "parent" (instead of "grid") property to the options array at the beginning of renderArray.

Index: List.js
===================================================================
--- List.js (revision 6021)
+++ List.js (working copy)
@@ -459,6 +459,8 @@
            //      Renders an array of objects as rows, before the given node.

            options = options || {};
+           options.parent = this;
+
            var self = this,
                start = options.start || 0,
                rowsFragment = document.createDocumentFragment(),

I have updated the fiddle here to show the results. I don't know how to change List.js for the fiddle without hosting it myself, so I just have hacked in a renderArray of my own that sets up the parent property before calling inherited.

Thoughts?

[schallm]

Looking at this a little further, I think it would actually be a better idea to inject the grid/this inside of Grid.js instead of List.js. List's don't need the additional property as they don't have columns. I also don't really like the property name of "parent" as it is ambiguous. If we make the following change, the property could actually be named "grid" and only affect Grid and it's descendants...

Index: Grid.js
===================================================================
--- Grid.js (revision 6021)
+++ Grid.js (working copy)
@@ -99,6 +99,12 @@
            }
        },

+       renderArray: function (results, beforeNode, options) {
+           options = options || {};
+           options.grid = this;
+           return this.inherited(arguments);
+       },
+
        createRowCells: function (tag, createCell, subRows, item, options) {
            // summary:
            //      Generates the grid for each row (used by renderHeader and and renderRow)

[edhager]

dgrid is not designed to share column definitions across grids. It is listed as a limitation in the wiki.

[edhager]

I pushed a change to commit 2d0e895d499a1386fa82dc924007e0ab1c9b1b27 that will cause the grid to produce a console warning if a column definition object is shared between grids.

[schallm]

Thanks for the pointer Ed. I did not see that limitation. Since I missed it, could I also suggest this be mentioned in the column definition documentation ?

[edhager]

I added the column definition limitations to the Grid documentation: https://github.com/SitePen/dgrid/blob/master/doc/components/core-components/Grid.md#column-definition-limitations

[schallm]

That looks great. Thanks!

I don't know the actual/full reason for the limitation, but this could be closed unless dylans still wants to add the grid property to the options object for the next release.

[dylans]

@schallm After thinking about it more, it is probably safer, simpler, and cleaner to employ some sort of factory pattern if you want to reuse column definitions across dgrid instances without side effects. The factory pattern would return a column definition instance that would then get returned to be used in the constructor for each dgrid instance. This would probably be cleaner than trying to track down all the possible side effects of sharing a column definition across dgrid instances. I'm thinking the column definition should really be a separate instance per dgrid to prevent unintended side effects in your codebase.

[schallm]

@dylans Agreed. We are now just passing a "new" columns array/object when calling new. Rather than creating a new type with columns array "baked in".