Avoid circular deps

[ivanvanderbyl]

Due to a limitation in the AMD module loader, d3-color cannot be compiled by Babel.js because it produces a build which doesn't include the circular reference to Color.prototype, causing all the additional color functions to fail.

Why am I compiling this to AMD? Because of Ember.js. Unfortunately UMD as output by this project cannot be loaded by Ember and there is are no plans to support anonymous module loaders, so the Ember Addon ember-cli-d3-primitive recompiles the ESNext sources to AMD using Babel.

[mbostock]

1. I’m not saying we can’t workaround the circular reference here, but did you also file a bug with Babel regarding circular references? It seems like it would be useful to fix this upstream.
2. Do you need to use Babel to compile your AMD bundle? You could use Rollup to generate an AMD, rather than UMD, bundle. Simply change -f umd to -f amd. (See Rollup documentation.)
3. I don’t understand the reference to “anonymous module loaders” in the description; the provided UMD module is not anonymous. In an AMD environment, it defines a module named “d3-color”.
4. The style changes are unrelated to this fix. Most of the diffs are style changes, so I’m going to close this PR and will make a separate branch removing the circular reference.

[mbostock]

Okay, I’ve created #23. Note that there was also a circular dependency between Lab and HCL colors. Curious if that affected you or if by luck it did not.

I feel like it’d be more robust to fix this in Babel, since there’s nothing here that would prevent a circular dependency from occurring again in the future (or in other D3 modules). But… it doesn’t seem that bad to consolidate the interdependent code, either.

[ivanvanderbyl]

Thanks for the feedback @mbostock, and sorry about the style changes. Let me address a few things here so you've got an idea of why this became an issue:

As I mentioned, I'm working on an Ember addon to bring the D3 v4 code in to Ember, which at the moment for v3 requires using Bower which we're trying to avoid as it will be deprecated in the future.

Ember doesn't support UMD modules because they are effectively anonymous to Ember's module loader, even though they declare support for AMD and declare themselves with a name, the internal packages are not named. Unfortunately Ember's module loader doesn't support all AMD features (See https://github.com/ember-cli/loader.js/issues/47), and one of the missing features is required by the AMD loader to work correctly, so it doesn't declare define.amd.

So this is why I set out building it myself from source. I guess I could have put a request in for all your v4 packages to include a Rollup build in AMD, but for all other packages building with babeljs worked great, and it produces builds which I can import only what I need, like a line from d3-shape/line instead of loading the whole package. Granted future compilers will remove dead paths any way, but this is more declarative.

As for the circular dependency issue in Babel, here's the reported issue: https://phabricator.babeljs.io/T6684, it notes version 6, but the issue exists in 5 as well.

I agree fixing babel would be better, but that's outside my league. How do you feel about producing an AMD build using rollup as well?

[mbostock]

Huh. Well, I guess my first impression is that Ember loader’s lack of support for UMD is causing pain (ember-cli/loader.js#4 ember-cli/loader.js#23 ember-cli/loader.js#47 ember-cli/loader.js#55 ember-cli/loader.js#60), so maybe it’s worth having the Ember loader handle at least named UMD’s automatically.

Admittedly I know almost nothing about the Ember loader, so I could be misinformed. But I see only the objection to anonymous modules, and these UMD’s are named. Perhaps it could work.

Short of that, it seems like you could either use a bundler that handles circular dependencies correctly and outputs AMD (such as Rollup with -f amd), or you could even take the UMD bundle provided in the release and simply strip the UMD header (the first four lines, say using tail -n+5 d3-color.js) to replace it with an Ember loader-compatible declaration. So, replace this:

(function (global, factory) {
  typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports) :
  typeof define === 'function' && define.amd ? define('d3-color', ['exports'], factory) :
  factory((global.d3_color = {}));

With this:

(function (global, factory) {
  define('d3-color', ['exports'], factory);

Or whatever works.

It wouldn’t be hard to add an AMD distribution to all D3 module releases, but I fear it would introduce confusion to users, who would have to understand the difference between d3-color.js and d3-color.amd.js. The radical idea behind UMD is that it works everywhere! (Though my actual experience with anything related to AMD and RequireJS is that it’s an unending timesuck…)

[ivanvanderbyl]

These changes have fixed the babel issue, thanks.

I'll have to experiment with the UMD build and see how loader.js interprets it. I originally tried making a shim for each package which invoked rollup to compile to AMD, but then decided the maintenance would be too much.

You are correct in saying loader.js causes a lot of pain, but it's designed the way it is for the specific use case of ember source code from ES6 to ES5 AMD. There's some nuance there. Especially because Ember's build pipeline (http://broccolijs.com/) compiles all app and vendor sources into concatenated trees which are diffed and merged as individual files are changed. The output of this is an AMD build of all sources regardless of whether they came from ES6 -> Babel -> AMD, AMD from Bower, or some other place. You can of course use globals, but it feels dirty.

Do you really think d3-color.amd.js would be confusing? I've seen a lot of other packages do this. It would negate the need to have an additional compile step in my addon which increases build time by about 3 seconds.

[mbostock]

Yes, I believe a d3-color.amd.js would be confusing.

Many people that use D3 are new to web development and programming in general. A very common usage pattern is a simple script tag, and I expect a good percentage of users don’t know what AMD is. There’s a slight mitigating factor that this is a D3 module (d3/d3-color) rather than the default build of D3 (mbostock/d3), and so there’s a slightly higher barrier to entry, but even so… I feel the presence of an AMD bundle in every D3 module merely to serve Ember loader isn’t the right trade-off.

I’m not entirely convinced that #23 is an improvement independent of being a workaround for this issue, but… it doesn’t seem bad to put tightly-coupled code in the same file, so I think I’m okay with merging it.

[ivanvanderbyl]

Just a late update to this, I've found a novel solution which should work for all packages moving forward: Using recast to rewrite the module definition to something which Ember can load. https://github.com/ivanvanderbyl/ember-cli-d3-primitive/blob/e823540dc2481592a8c3bd2b7ae3347554d2edc0/lib/rewrite-amd-definition.js#L41-L59

[mbostock]

:+1: