DataTable - cleanup - fragile code

[kum-deepak]

_doColumnHeaderFnToString (https://github.com/dc-js/dc.js/blob/develop/src/charts/data-table.js#L92) is quite fragile.

dc.js/src/charts/data-table.js

Lines 92 to 107 in 82469c9

	_doColumnHeaderFnToString (f) {
	// columnString(f) {
	let s = String(f);
	const i1 = s.indexOf('return ');
	if (i1 >= 0) {
	const i2 = s.lastIndexOf(';');
	if (i2 >= 0) {
	s = s.substring(i1 + 7, i2);
	const i3 = s.indexOf('numberFormat');
	if (i3 >= 0) {
	s = s.replace('numberFormat', '');
	}
	}
	}
	return s;
	}

It is there to maintain backward compatibility in the columns argument.

Currently, the chart supports three different ways (http://dc-js.github.io/dc.js/docs/html/DataTable.html#columns__anchor) to specify columns. I think we can drop the only functions option and stick to the other two. Even though possible to maintain current behavior in the compat layer, since the code is quite fragile we should not offer backward compatibility.

[gordonwoodhull]

I agree this is horrible code and should be removed.

The story is that the original .columns() only took an array of functions, and it didn't generate the table header automatically. At that time, the user had to provide a table element with headers already in it.

When automatic generation of the header was added in #560 / #668, the author added this code to allow a mixture of functions, string field names, and full column definitions. I remember discussing this fragile code, and the author pointing out that it does work across browsers, but I can't find that discussion now.

Note that the all-function case is special:

dc.js/src/charts/data-table.js

Lines 117 to 122 in 82469c9

	let bAllFunctions = true;
	this._columns.forEach(f => {
	bAllFunctions = bAllFunctions & (typeof f === 'function');
	});
	if (!bAllFunctions) {

(could be cleaned up with Array.every)

I'd prefer to deprecate and move the fragile code to the compatibility layer, but if you can't conscience that, we could disallow functions when there is a mix while still preserving compatibility with old examples. Hopefully the mixed case isn't used all that much.

[mr23]

I'm all for deprecate and replace with good design. 560/668 was just trying to make programmability possible without the needed rewrite. Chris aka mr23 aka ChrisMeierRT

[gordonwoodhull]

Thanks @mr23, the design for column headers has held up really well over all these years. It's just the reading of the text of the function that is fragile. I agree that there's no other way to automatically determine the column name from just a function.

Well, there is one other way: if the functions aren't arrow functions, they could have names, like

dataTable.columns([
    function date(d) { return d.date; },
    function open(d) { return d.open; },
    function close(d) { return d.close; },
    function change(d) { return numberFormat(d.close - d.open); },
    {
      name: 'volume',
      format: d => d.volume;
    }
]);

I don't know if it's worth pursuing - it's not backward-compatible but it allows another terse way to specify columns.

[kum-deepak]

We should recommend the following syntax, which is explict at the same quite succint:

chart.columns([
    "date",    // d["date"], ie, a field accessor; capitalized automatically
    "open",    // ...
    "close",   // ...
    {
        label: "Change",
        format: function (d) {
            return numberFormat(d.close - d.open);
        }
    },
    "volume"   // d["volume"], ie, a field accessor; capitalized automatically
]);

The other alternate we should move to the compat layer. I will do a PR when I through with the current round of typings.