Always expose D3 onto the environment global #1921

Merged
merged 1 commit into from Jul 12, 2014

Conversation

4 participants
@tbranyen
Contributor

tbranyen commented Jun 24, 2014

With regards to #1689 adding AMD compatibility, it seemed odd to me that
D3 opt'd out of exposing itself globally for third-party plugins.

Within a discussion about this very problem, found here:
mpld3/mpld3#33 (comment)

the argument is made that exposing the global defeats the point of
module systems. I agree that module systems help avoid global
pollution, but that is not something I'd consider the point of module
systems, especially since they are registered in a global namespace
themselves.

This patch will fix third-party modules that are shimmed.

Always expose D3 onto the environment global
With regards to #1689 adding AMD compatibility, it seemed odd to me that
D3 opt'd out of exposing itself globally for third-party plugins.

Within a discussion about this very problem, found here:
mpld3/mpld3#33 (comment)

the argument is made that exposing the global defeats the point of
module systems.  I agree that module systems help avoid global
pollution, but that is not something I'd consider the point of module
systems, especially since they are registered in a global namespace
themselves.

This patch will fix third-party modules that are shimmed.
@mbostock

This comment has been minimized.

Show comment
Hide comment
@mbostock

mbostock Jul 12, 2014

Member

Man, AMD support really opened up a can of worms, and I wonder if I regret it.

It seems to me like the main benefit of D3 supporting AMD is that it doesn’t define the global. If you don’t care about defining the global, then why support AMD at all? Just let people configure a shim and load D3 like every other JavaScript library.

Related discussion:

https://github.com/jrburke/requirejs/wiki/Updating-existing-libraries

My take away is that jQuery is “special”, but I don’t know whether D3 is special in the same way that jQuery is special.

Like jQuery, D3 is also a “foundation of other third party libraries that all assume a global is available”. So should D3 provide a global? The problem with D3 not providing a global is that every D3 plugin (also see d3/d3-geo-projection#9) must support AMD, or the plugin breaks because of a ReferenceError on the d3 global, or you force the user to jump through some tedious RequireJS config hoops to export the d3 global manually.

Like jQuery, D3 is “commonly used as a dependency by other libraries” and D3 “is also the defacto implementation of the module interface implied by that name”. So should D3 register as a named module, too? I have no idea. The discussion suggests that in the future jQuery will switch to an anonymous module.

So I guess our options are:

  1. Do nothing, and recommend that users specify whatever elaborate config is required to export the d3 global before loading a plugin. This is a documentation-only change, so perhaps we should do this regardless before we do anything else.
  2. Do nothing, and recommend that all D3 plugins adopt AMD. This is the most pro-AMD, but also will inflict the most pain on the user, since there’s no practical way to get every D3 plugin to add AMD support. And it’s tedious for developers. I’ve done this for some of my other libraries (queue and topojson), but I haven’t implemented AMD support for all D3 plugins. (Again, see d3/d3-geo-projection#9 for an example.)
  3. Remove AMD support from the default build. This would break everyone loading d3.v3[.min].js via AMD, so it’d probably need to be done as part of the 4.x release. We could still provide a separate build with AMD support (say d3.v4.amd.js) that is identical but provides whatever magic incantation is preferred by AMD. (We could also remove support for module.exports, since that is trivially supported using package.json or browserify.json and a wrapper script, as we were doing previously…)
  4. Export the d3 global. This is unlikely to cause additional harm, although it’s conceivable that it could clobber something in the global namespace (such as an earlier version of D3 on the same page). This would just make any D3 plugins that do not support AMD start working again, with no fuss for the user. But it is the most unclean approach since we would dirty the global namespace, even though we claim to support AMD.

Options 1 & 2 aren’t exclusive, and would be most in continuing with our existing approach.

Option 3 would keep AMD separate from the main library and allow different options tailored to whatever people need, and perhaps I could stop wasting so much time thinking about how to load a JavaScript file. 👿

Option 4 is arguably the best for users, regardless of whether they are using AMD, though it’s a weak compromise that doesn’t establish a clear policy for other libraries. (Should queue and topojson export a global too?) Also, it’s still possible for advanced users to create their own custom build if they don’t want the global, or if they want to load D3 as a named module.

So I guess I’ve convinced myself to agree with you and export a global. And I’ll probably change my other libraries to do the same. For one thing, this will make me happy on nytimes.com because d3 will finally work in the console again…

Member

mbostock commented Jul 12, 2014

Man, AMD support really opened up a can of worms, and I wonder if I regret it.

It seems to me like the main benefit of D3 supporting AMD is that it doesn’t define the global. If you don’t care about defining the global, then why support AMD at all? Just let people configure a shim and load D3 like every other JavaScript library.

Related discussion:

https://github.com/jrburke/requirejs/wiki/Updating-existing-libraries

My take away is that jQuery is “special”, but I don’t know whether D3 is special in the same way that jQuery is special.

Like jQuery, D3 is also a “foundation of other third party libraries that all assume a global is available”. So should D3 provide a global? The problem with D3 not providing a global is that every D3 plugin (also see d3/d3-geo-projection#9) must support AMD, or the plugin breaks because of a ReferenceError on the d3 global, or you force the user to jump through some tedious RequireJS config hoops to export the d3 global manually.

Like jQuery, D3 is “commonly used as a dependency by other libraries” and D3 “is also the defacto implementation of the module interface implied by that name”. So should D3 register as a named module, too? I have no idea. The discussion suggests that in the future jQuery will switch to an anonymous module.

So I guess our options are:

  1. Do nothing, and recommend that users specify whatever elaborate config is required to export the d3 global before loading a plugin. This is a documentation-only change, so perhaps we should do this regardless before we do anything else.
  2. Do nothing, and recommend that all D3 plugins adopt AMD. This is the most pro-AMD, but also will inflict the most pain on the user, since there’s no practical way to get every D3 plugin to add AMD support. And it’s tedious for developers. I’ve done this for some of my other libraries (queue and topojson), but I haven’t implemented AMD support for all D3 plugins. (Again, see d3/d3-geo-projection#9 for an example.)
  3. Remove AMD support from the default build. This would break everyone loading d3.v3[.min].js via AMD, so it’d probably need to be done as part of the 4.x release. We could still provide a separate build with AMD support (say d3.v4.amd.js) that is identical but provides whatever magic incantation is preferred by AMD. (We could also remove support for module.exports, since that is trivially supported using package.json or browserify.json and a wrapper script, as we were doing previously…)
  4. Export the d3 global. This is unlikely to cause additional harm, although it’s conceivable that it could clobber something in the global namespace (such as an earlier version of D3 on the same page). This would just make any D3 plugins that do not support AMD start working again, with no fuss for the user. But it is the most unclean approach since we would dirty the global namespace, even though we claim to support AMD.

Options 1 & 2 aren’t exclusive, and would be most in continuing with our existing approach.

Option 3 would keep AMD separate from the main library and allow different options tailored to whatever people need, and perhaps I could stop wasting so much time thinking about how to load a JavaScript file. 👿

Option 4 is arguably the best for users, regardless of whether they are using AMD, though it’s a weak compromise that doesn’t establish a clear policy for other libraries. (Should queue and topojson export a global too?) Also, it’s still possible for advanced users to create their own custom build if they don’t want the global, or if they want to load D3 as a named module.

So I guess I’ve convinced myself to agree with you and export a global. And I’ll probably change my other libraries to do the same. For one thing, this will make me happy on nytimes.com because d3 will finally work in the console again…

@mbostock mbostock merged commit 7f2d63e into d3:master Jul 12, 2014

@tbranyen tbranyen deleted the tbranyen:expose-global branch Jul 12, 2014

@tbranyen

This comment has been minimized.

Show comment
Hide comment
@tbranyen

tbranyen Jul 12, 2014

Contributor

Hey @mbostock thanks for taking the time for a well thought out response. I'm glad you merged this PR as it will fix a lot of headaches and confusion I've shared. I have some comments below to address your options:

  1. While documentation certainly helps, libraries should be designed to the least surprise. If you export for all environments where possible, your tool is available to a wider audience with less fuss. You mention this as well, so we're on the same page.
  2. I wouldn't recommend making AMD a requirement. Especially since d3 exports correctly in all major module environments and ES6 may-or-may-not be around the corner. I believe with this PR merged, you have the library exporting as-good-as-it-gets.
  3. Removing AMD seems like the wrong way to go. Too much flux and pretty much all mainstream libraries have adopted exporting as d3 does. Its become a standard expectation.

The new ES6 modules specification has almost no momentum, but hopefully it will address the pain in even thinking about this stuff. I can sympathize too; I've implemented UMD incorrectly several times trying to be clever and hating the whole decoration necessity.

Contributor

tbranyen commented Jul 12, 2014

Hey @mbostock thanks for taking the time for a well thought out response. I'm glad you merged this PR as it will fix a lot of headaches and confusion I've shared. I have some comments below to address your options:

  1. While documentation certainly helps, libraries should be designed to the least surprise. If you export for all environments where possible, your tool is available to a wider audience with less fuss. You mention this as well, so we're on the same page.
  2. I wouldn't recommend making AMD a requirement. Especially since d3 exports correctly in all major module environments and ES6 may-or-may-not be around the corner. I believe with this PR merged, you have the library exporting as-good-as-it-gets.
  3. Removing AMD seems like the wrong way to go. Too much flux and pretty much all mainstream libraries have adopted exporting as d3 does. Its become a standard expectation.

The new ES6 modules specification has almost no momentum, but hopefully it will address the pain in even thinking about this stuff. I can sympathize too; I've implemented UMD incorrectly several times trying to be clever and hating the whole decoration necessity.

@sheppard sheppard referenced this pull request in Leaflet/Leaflet Jul 23, 2014

Closed

modules, globals, and plugins #2364

@sheppard

This comment has been minimized.

Show comment
Hide comment
@sheppard

sheppard Jul 23, 2014

Aww, I was hoping for Option 2 to become adopted as a "standard" approach so I could stop having to add AMD wrappers to plugins myself. Oh well, a global d3 isn't the end of the world. FWIW, even under AMD you could always do a synchronous

window.d3=require('d3');

in the console as long as d3 has already been loaded once asynchronously.

Aww, I was hoping for Option 2 to become adopted as a "standard" approach so I could stop having to add AMD wrappers to plugins myself. Oh well, a global d3 isn't the end of the world. FWIW, even under AMD you could always do a synchronous

window.d3=require('d3');

in the console as long as d3 has already been loaded once asynchronously.

@Vanuan

This comment has been minimized.

Show comment
Hide comment
@Vanuan

Vanuan Mar 11, 2017

The new ES6 modules specification has almost no momentum

It's no longer the case.

Vanuan commented Mar 11, 2017

The new ES6 modules specification has almost no momentum

It's no longer the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment