-
Notifications
You must be signed in to change notification settings - Fork 87
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
CommonJS Support #113
base: master
Are you sure you want to change the base?
CommonJS Support #113
Conversation
I'm using this library with Browserify and found that it was unable to detect d3 correctly. This adds in the necessary `require` call to find the d3 dependency. I also changed the UMD signature to not expect that `define` and `require` are globals in the sense of the runtime, but instead are global within the encapsulating function scope (which may be the same as the runtime's). This will ensure d3.chart works as expected when bundled.
Use Browserify (courtesy of the "grunt-browserify" module) to build a "test application" to ensure that this library properly functions in CommonJS environments.
Hey @jugglinmike not sure why GitHub keeps failing at delivering notifications to me about projects I'm mentioned in, as I just stumbled across this accidentally. I left it out because my changes were specifically exporting the correct interface to be consumed by any CJS implementation. I wasn't thinking npm/browserify/node necessarily. I don't really understand the reasoning behind npm's decision to effectively deprecate |
@jugglinmike Is there a reason this hasn't been merged in yet? Adding UMD support would be really useful for me. I have to do some really strange things right now to pull it in to a webpack build reliably. If there are any issues with this pull request I would be happy to help get them resolved. |
👍 |
I've implemented a better solution that improves the codebase's internals as well. Also, I'm thinking @iros and I will try to get this out this week. (Heads up: we'll need a new minor version) |
@jugglinmike Is this solution up on a branch? |
I'm using this from https://github.com/danemacaulay/d3.chart with success. |
Add tests for the improvement submitted by @tbranyen gh-110.
Tim: I've formally declared d3 as a dependency in
package.json
. Because of the upcoming changes topeerDependencies
, I find myself doubting how to express this relationship. Did you intentionally leave that part out?