Chart constructor support in API

[knuton]

I'd like to propose a couple of non-breaking API changes for v0.2(.x). Both concern the handling of chart constructors.

As expressed in #62, one of the goals for v0.2 is committing to constructors. It's great to have a formal commitment already, but I think the API could be easily adapted to further improve support for them. These changes should address some of the issues discussed in #41.

I propose the following changes:

1. Allow registration of chart constructors via d3.chart:

   d3.chart = function(name) {
     if (arguments.length === 0) {
       return Chart;
     } else if (arguments.length === 1) {
       return Chart[name];
     // register chart if name and `Chart` subclass are given
     } else if (arguments[1].prototype instanceof Chart) {
       return Chart[name] = arguments[1];
     }
   
     return Chart.extend.apply(Chart, arguments);
   };

   This allows to make readymade charts available under a name that makes sense to the respective project. In practice this is already possible through d3.chart()['SpecialChart'] = SpecialChart, but official API support would be nice.

2. Accept chart constructor references in d3.selection.prototype.chart:

   d3.selection.prototype.chart = function(chartOrChartName, options) {
     // Without an argument, attempt to resolve the current selection's
     // containing d3.chart.
     if (arguments.length === 0) {
       return this._chart;
     }
     // Check for chart constructor
     if (chartOrChartName.prototype instanceof Chart) {
       return new chartOrChartName(this, options);
     }
     var ChartCtor = Chart[chartOrChartName];
     d3cAssert(ChartCtor, "No chart registered with name '" + chartOrChartName + "'");
   
     return new ChartCtor(this, options);
   };

   This allows to conveniently use chart components from the d3 API without storing everything in the d3 object. It is just as concise as using a chart name.

3. Make chart name optional in Chart.extend, i.e. if first argument (name) is an object instead of a string, shift arguments accordingly and simply don't store a reference to the new chart in Chart[name]. This would allow the convenient creation of not globally registered chart constructors. @jugglinmike, I saw in Specifying charts without using a name #41 that you were worried about library users creating anonymous charts directly via d3.selection.prototype.chart, which I think is a valid concern. So I would prohibit that, and only allow creation of anonymous charts via d3.chart and Chart.extend.

What do you think? Any oversights?

[jugglinmike]

Hey there @knuton,

First off, thanks for your interest in d3.chart! You've clearly done your
homework in designing these modifications, and I very much appreciate that.

All three of your suggestions attempt to extend the "D3.js-like" APIs with
prototypal inheritance patterns. The arguments inference this requires has
the unfortunate side-effect of making those methods harder to understand and
harder to document. Since one of our goals is to make this library more
approachable, I think this may be the wrong approach.

Your intention ("Chart constructor support") is very well-appreciated, though.
Here are my thoughts:

D3.js's API abstracts many of the details relating to classes and object
instantiation. This library was designed with the belief that access to those
primitives is fundamental to reusable code (and may be part of the reason why
visualizations authored with D3.js so rarely get reused).

Instead of trying to extend the current "D3.js-like" API with support for more
nuanced techniques (i.e. constructors), what do you think about exposing those
APIs directly? From there, we could build the high-level API on top.

var chartNs = {};

d3.chart.register = function(name, ChartCtor) {
  return chartNs[name] = ChartCtor;
};

// d3.chart
// A factory for creating chart constructors and retrieving registered chart
// constructors.
d3.chart = function(name, protoProps, staticProps) {
  var ChartCtor;
  if (arguments.length === 0) {
    return Chart;
  } else if (arguments.length === 1) {
    return chartNs[name];
  }

  ChartCtor = Chart.extend(protoProps, staticProps);
  d3.chart.register(name, ChartCtor);

  return ChartCtor;
};

// d3.Chart
// A constructor for an "empty" chart. Generally, you will want to define new
// Chart constructors using the `d3.Chart.exted` method.
d3.Chart = Chart;

   // Set a convenience property in case the parent's prototype is needed
   // later.
   child.__super__ = parent.prototype;

+  d3.chart.register(name, child);
-  Chart[name] = child;
   return child;
 };

This is the practical effect on your first two use cases:

  // #1 - constructor registration
+ d3.chart.register("MyChart", MyChart);
- d3.chart("MyChart", MyChart);

  // #2 - Chart instantiation
+ new MyChart(d3.select("body"), { option: 23 });
- d3.select("body").chart(MyChart, { option: 23 });

Unfortunately, the separation of "d3-like" API and "plain JavaScript" API is
not so simple for the extend method. This method would be shared by users
using both styles. I think we might be forced to play "arguments hockey" as
you've recommended in #3...

Thoughts?

[knuton]

Of course I was a little proud of having sneaked everything nicely into the existing API. But I think you are right, it should be easier for library users if d3.chart supports both paradigms separately and everything has its place. I think your suggestion elegantly achieves that.

As for the case of extend, I suppose extending could be decoupled from registration, but this would make it more cumbersome to use. Maybe a little bit of magic is okay here. We don't actually need this ourselves, as we rely on CoffeeScript's inheritance mechanism. (Which by the way works great with d3.chart. Is this a deliberate design decision? Will it continue to have support?)

One more thought. After your proposed change there are both d3.chart and d3.Chart, which might be slightly confusing. Maybe the constructor could live at d3.chart.Chart. If that looks silly, maybe it could live there under an altered name such as d3.chart.BasicChart. But I don't actually think it looks too silly.

Thanks!

[knuton]

How should we go on, should I prepare a patch for it so we can see it in action?

[iros]

It seems that there are many approaches to this problem. Not all my charts,
for example require margins. Because d3.chart.base aims to be the lowest
common denominator, at this time I do not foresee adding any specific
margin convention to it.

Thanks, @iros

On Feb 23, 2014, at 5:56 AM, Johannes Emerich notifications@github.com
wrote:

How should we go on, should I prepare a patch for it so we can see it in
action?

Reply to this email directly or view it on
GitHubhttps://github.com//issues/76#issuecomment-35828954
.

[knuton]

@iros, this is another issue, seems like you write in response to #81? ;)

[iros]

Yes. Apologies. Replying via email without thread context is clearly a bad
idea :)

On Sunday, February 23, 2014, Johannes Emerich notifications@github.com
wrote:

@iros https://github.com/iros, this is another issue, seems like you
write in response to #81#81?
;)

Reply to this email directly or view it on GitHubhttps://github.com//issues/76#issuecomment-35831568
.

Excuse my brevity, typed on the go.

[jugglinmike]

Let's formalize the open questions:

1. How should the Chart constructor be exposed? This is pretty trivial, but we
   should get it right the first time. Options:

   • d3.chart.Chart
   • d3.chart.BasicChart
   • d3.Chart
   • More, I'm sure. Nothing particularly appealing about any of the above,
     although I'm still partial to the last.

   We can also use the proposed d3.chart.register API to expose the
   constructor, as in d3.chart("Chart"), but I would like to see that in
   addition to a static reference (not in place of one).

2. How should we support naming charts through extend?

   • Use our current solution. This will force those using constructors directly
     to supply a name when extending existing charts which is inconsistent with
     how they create basic charts:

     var Fancy = Chart.extend({ /* options */ });
     var ReallyFancy = Fancy.extend("ReallyFancy", { /* options */ });

   • Remove the "name" parameter. This will force those using the d3-like API to
     make an additional call to d3.chart.register which is inconsistent with
     how they create basic charts:

     d3.chart("Fancy", { /* options */ });
     var ReallyFancy = d3.chart("Fancy").extend({ /* options */ });
     d3.chart.register("ReallyFancy", ReallyFancy);

   • Use arguments inference to change the behavior of extend based on the
     style being used. This makes constructor-based code happy:

     var Fancy = Chart.extend({ /* options */ });
     var ReallyFancy = Fancy.extend({ /* options */ });

     and also supports d3-like users well:

     d3.chart("Fancy", { /* options */ });
     d3.chart("Fancy").extend("ReallyFancy", { /* options */ });

   That last option seems nicest for users but only once they've learned it. The
   JSDoc would read something like this:

   /**
   * Create a new {@link Chart} constructor with the provided options acting as
   * "overrides" for the default chart instance methods. Allows for basic
   * inheritance so that new chart constructors may be defined in terms of
   * existing chart constructors. Based on the `extend` function defined by
   * {@link http://backbonejs.org/|Backbone.js}.
   *
   * @static
   *
   - * @param {String} name Identifier for the new Chart constructor.
   + * @param {String} [name] Optional identifier for the new Chart constructor.
   * @param {Object} protoProps Properties to set on the new chart's prototype.
   * @param {Object} staticProps Properties to set on the chart constructor
   *        itself.
   *
   * @returns {Function} A new Chart constructor
   */

   ...which, in retrospect, really isn't that bad at all...

@knuton @iros I'm still not sure about #1, maybe we could brainstorm a few
more options? As for #2, I'm coming around to that third approach.

[knuton]

1. Agreed that a static reference should be present. Out of the three present options, I like d3.chart.Chart best. First, because it's less likely to cause accidental referencing of the wrong entity, but second also because it minimizes "pollution" of the d3 global. Related to the latter, if in the future you want to provide support for module systems (CommonJS, AMD), you can simply export d3.chart.

   (And look at d3.chart.Chart with the eyes of a Java programmer. It looks only natural!)

   Would be interested in your reasoning for d3.Chart.

2. Docs and examples for arguments inference look good to me, I think it nicely envelops both approaches.