-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid circular deps #22
Conversation
|
There’s a circular relationship between the generic color parser and the rgb and hsl constructors: the color parser instantiates an rgb or hsl color with the parsed channel values, and the rgb and hsl constructors (when called with a single argument) applies the color parser to first create an RGB color. There’s also a circular relationship between the Lab and HCL color spaces, since the latter is a cylindrical transformation of the former. Some module loaders (notably Babel; see #22) apparently cannot handle circular dependencies between files. So now the files are merged.
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. |
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 ember-cli/loader.js#47), and one of the missing features is required by the AMD loader to work correctly, so it doesn't declare 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 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? |
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 (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…) |
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 Do you really think |
Yes, I believe a 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. |
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 |
👍 |
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 toColor.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.