CommonJS Support (with refactored internals)

[jugglinmike]

I've always wanted to express internal dependencies in a more formal way while
preserving the ability to test the source files directly (i.e. no build step,
no testing built files). AMD was the clear choice here, but at the time, it was
difficult to package library code that used define without running into
problems when consumer code also used AMD.

@tbranyen and I made a few good runs at a solution, but we never finalized a
tool that we felt was production-ready. In the time since, the Webpack project
has been released with support for exactly this use case.

A nice side-effect is that we get CommonJS support (previously proposed in
gh-110) for free.

At first, I thought this change would require a minor version increase, but I'm
not so sure of that anymore. There are a couple minor differences for
developers consuming d3.chart from AMD-enabled environments, but neither can
really be considered "breaking":

• Previously, the AMD "shim" config was necessary to properly load d3.chart.
  This patch makes existing configurations superfluous.
• Previously, the library did not export a value. This patch exports the more
  useful function attached to the d3 API as d3.chart.

...but it's been so long since our last minor release, it might be best to play
it safe and publish this as 0.3.0. @tbranyen: if you have a minute, could you
let me know if that sounds right to you?

I've split this work into two separate commits: the first refactors internals,
and the second tests CommonJS support. I'm hoping this will help with code
review because testing CommonJS support was not trivial, so a distinct commit
more clearly delineates my efforts towards those ends.

As always, I'd love from feedback from any of the project's consumers. In this
case, I'm particularly interested in respondents to gh-113. @JMStewart,
@nicksrandall, @Aharris88, @wvengen: Does this look good to you folks?

[wvengen]

Thanks! Looking good at first glance. Using the exported value (instead of d3.chart) feels more natural. I'll start testing it in a real-world project in the next week.

[wvengen]

I think it would be a good idea to include an example for a depending chart (like a barchart module based on d3.chart). With webpack, I had to declare my dependencies something like this:

// webpack.config.js
// ...
module.exports = {
  // ...
  externals: {
    'd3': true,
    'd3.chart': {
      'root': ['d3', 'chart'],
      'commonjs':'d3.chart',
      'commonjs2': 'd3.chart',
      'amd': 'd3.chart' /* to check */
     }
  }
};

Then I could create a new chart like this:

var d3 = require('d3');
var Chart = require('d3.chart');

Chart('MyChart', {
  // ...
});

module.exports = Chart;

[jugglinmike]

@wvengen Thanks for kicking the tires here!

While I'm happy to include general usage guidelines for AMD and CommonJS, I'm reluctant to document usage for specific tools. There are a lot of module loaders out there, and I want to avoid having to make subjective decisions about which ones need documentation. More than that, I don't want to be in a situation where d3.chart's documentation of another tool falls out-of-date. If d3.chart exposes a package that conforms to the (semi) standards, then I think it's fair to expect consumers to refer to the documentation of their module loader of choice.

That said, the configuration you've shared seems kind of heavy for such a fundamental use case. I'm not very experienced with Webpack, though; can you advise if there is anything we could change about this patch to make d3.chart more readily consumable with that tool?

[JMStewart]

This looks pretty good to me. I pulled it into some of my projects and it is working great. I like having the exported value for creating new charts, as @wvengen showed above. I played around with this a bit, and found I can also instantiate a chart like this:

var d3 = require('d3');
var Chart = require('d3.chart');
var MyChart = require('MyChart');

var g = d3.select('svg')
  .append('g');

var myChart = new MyChart(g);

As for the webpack configuration, the only thing making it complicated is that d3.chart attaches itself to d3, and doesn't create it's own global variable on the root when pulled in without a loader. I think the only way to make it simpler would be to add a global variable when not used with a loader, although it would still have to be the same as the CommonJS and AMD name. So in this case you would probably have to add a 'd3.chart' global to the window, which is possible, but super weird. The only was to access it would be window['d3.chart'] so I really don't think it would make sense to do it. My opinion is that the way it is now is fine, and actually just requires the same amount of configuration as libraries like lodash and underscore.

[wvengen]

@jugglinmike sounds good; not being too familiar with webpack exports configuration, it took me quite a while to figure out what to use exactly. Some general pointers like you suggest are fine.

This webpack definition is for chart libraries depending on webpack. Apps generally use just a single loading type, so should be fine with either d3.chart or ['d3', 'chart']. I think (not being too familiar with webpack either). @JMStewart sums it up quite nicely, I think. I have no better ideas.

👍 for using creating charts using a chart's require value new MyChart(g).

[wvengen]

I'm afraid there's still a problem with defining new charts in this way. I've put together a basic example that shows the error at https://gist.github.com/wvengen/5789e5b2a295156ea48e.

Error: TypeError: selection.layer is not a function
Defined in: layer-extensions.js

That's because we're using d3 without d3.chart to get the element.

edit updated the gist, it works now, thanks to @JMStewart

[JMStewart]

@wvengen It took me quite a while, but I finally figured out what was happening in your gist. The problem was that when you built d3.chart you installed it's dependencies, including d3. This resulted in your final webpack build including 2 copies of d3, with d3.chart only attached to one of them. Here's my fork of your gist with a couple changes, and it is working great for me. https://gist.github.com/JMStewart/3bc33e9c455581cf0f57

[jugglinmike]

Bravo, @JMStewart! The "build d3.chart yourself" step will go away when we release the next version and re-build the d3.chart.js file located in the root of the project.

As for removing node_modules, I'm not sure why this is necessary. This project does not specify any "production" dependencies of its own, so npm install d3.chart@git://github.com/jugglinmike/d3.chart#amd-3 (or one day, just npm install d3.chart) should produce

[jugglinmike]

...a node_modules/d3.chart/ directory without a node_modules/ directory of its own. Can you share how you are installing d3.chart? And maybe also the version npm you are using?

[JMStewart]

Yeah everything should work fine when this goes to production and we don't have to run the build step. npm install d3.chart will produce exactly what you describe, without a node_modules/d3.chart/node_modules/ folder. @wvengen's gist had this line to build d3.chart: cd node_modules/d3.chart && npm install && grunt build && cd - which installed all dev dependencies, including d3, into the local node_modules folder. I just changed that line to cd node_modules/d3.chart && npm install && grunt build && rm -rf node_modules && cd - to clean up after the build.

[jugglinmike]

Ah, got it.

[wvengen]

That was tricky. Thanks, @JMStewart, for tackling it! And it seems to work :)

[nicksrandall]

I really like the direction this is going. One small question, if we're using Webpack to bunlde d3.chart as a UMD module, is there a reason we're not setting the "output.library" and "output.umdNamedDefine" settings?

[wvengen]

@nicksrandall sounds like a good idea, I suppose something like this in webpack.config.js:

output: {
    libraryTarget: "umd",
    library: "d3.chart",
    umdNamedDefine: true
}

right?

[nicksrandall]

@wvengen Yes. This comes down to preference but I would set the libarary property as d3_chart so that it could be used as a global without having to access it like window['d3.chart'] as @JMStewart mentioned above. It's not a huge issue because most people won't use it that way.

FWIW, D3 v4's modules are following a similar pattern. For example, d3-shape module exports a d3_shape variable to the global scope. https://github.com/d3/d3-shape#installing

[wvengen]

Thanks for explaining! @jugglinmike would you care to add this? And perhaps then it's close to being ready for merging?

[jugglinmike]

Thanks for the feedback, you two! I think those changes make sense. I'm not opposed to renaming the browser global to d3_chart, either. The fact that it matches a convention in the up-and-coming major release of D3.js is nice, but I'm reluctant to put too much confidence in that convention until version 4 is officially released. I think the argument against window['d3.chart'] is more compelling. I very much want to promote the use of a module system, but I don't want to needlessly punish those using browser globals, either. (Though the exported value in those contexts is less important because d3 will be defined globally, making the "traditional" d3.chart the easiest choice.)

This is an straightforward change, but I'm a little swamped at the moment. I'll try to report back by this coming Friday.

[wvengen]

👍 thanks!

[jugglinmike]

Alrighty. I've pushed up a few changes. This differs a bit from what I planned
on in that the browser global is exposed as window["d3.chart"]. As far as I
can tell, WebPack does not expose dedicated configuration values for a
package's name as an AMD module and its name as a browser global. Although
require("d3_chart") isn't as awkward as window["d3.chart"], I think using
d3_chart in AMD is sufficiently confusing to warrant the unfortunate window
property name. So this may be a "punishment" for users relying on global
variables, the "needlessly" part of my first statement doesn't apply.

We could resolve this with a patch to Webpack itself, but I think there's
good reason to keep names consistent. And remember: the preferred reference in
those contexts, d3.chart, is ergonomic.

Seem fair to you folks?

[wvengen]

Great! I don't really mind the browser global name, and see both being used. I'd be happy with this. We can always add a d3_chart export when this settles in with d3js v4 (the other way around is less smooth).

I've tested 35cc230 with the gist and d3.chart.sankey succesfully.

[JMStewart]

Sounds good to me! I don't think think the global name is important enough to really worry about it.

[agarrharr]

Looks good to me and I'm fine with window["d3.chart"] for the global name. I'm excited to see this in a release soon! @jugglinmike Thanks for working on this!

[nicksrandall]

Sounds good to me! @jugglinmike when can we expect you to cut a release? Thanks again for you help on this.

[jugglinmike]

I just spoke with @iros about this, and she spotted some tweaks that I looked over. You can review the most recent commits in this pull request if you're curious, but basically:

• Update source code indentation to respect the AMD factory function
• Remove the hasOwnProp module in favor of accessing the built-in directly
• Update the project's readme.md file to document the newly-supported usage scenarios

I think these improvements are pretty non-controversial, but I'm still loathe to publish anything late at night. I'll be rebasing, merging, and releasing by Saturday (barring any objection from you folks).

Thanks for sticking with me!

[iros]

+1 to merge!

[JMStewart]

No objections from me. Thanks!

[agarrharr]

Looks good to me.

[jugglinmike]

Alrighty: I've squashed these changes, merged them to master, tagged a new release (updating the changelog and migration guide as well), and published to npm.

Thanks for the help, everybody!

[wvengen]

Works like a charm. Thanks!!

[tbranyen]

👏 nice work!