Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

CommonJS Support #113

wants to merge 3 commits into from

Conversation

jugglinmike
Copy link
Member

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 to peerDependencies, I find myself doubting how to express this relationship. Did you intentionally leave that part out?

tbranyen and others added 3 commits July 5, 2015 15:49
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.
@tbranyen
Copy link

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 peerDependencies, but maybe it's not something to worry about until npm v3 is stable?

@JMStewart
Copy link

@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.

@nicksrandall
Copy link

👍

@jugglinmike
Copy link
Member Author

I've implemented a better solution that improves the codebase's internals as well. Also, I'm thinking peerDependencies is the way to go here.

@iros and I will try to get this out this week. (Heads up: we'll need a new minor version)

@agarrharr
Copy link

@jugglinmike Is this solution up on a branch?

@wvengen
Copy link

wvengen commented Dec 18, 2015

I'm using this from https://github.com/danemacaulay/d3.chart with success.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants