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

Added support for loading D3 v4 #8

Merged
merged 38 commits into from
Oct 16, 2016
Merged

Added support for loading D3 v4 #8

merged 38 commits into from
Oct 16, 2016

Conversation

ivanvanderbyl
Copy link
Owner

Related discussion: #4

This is an almost verbatim import from https://github.com/ivanvanderbyl/ember-cli-d3-shape

// a dependency of another package.
if (!app.import) {
if (this.isDevelopingAddon()) {
console.log('[ember-cli-d3-shape] skipping included hook for', app.name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may break on older versions of node, which don't have a console global. Prefer to use
https://ember-cli.com/api/classes/UI.html#method_write

@mike-north
Copy link
Collaborator

@gtb104
Copy link

gtb104 commented Oct 3, 2016

I've been importing the entire D3 4.2.2 library, but in an effort to reduce the weight of our app, I wanted to pull in specific submodules. I was about to do what you are in the process of doing. ;-)

Is there a target date for this to be merged to master?

@ivanvanderbyl
Copy link
Owner Author

ivanvanderbyl commented Oct 4, 2016

@gtb104 if you can give this branch a try out in your app and report back that'd be a big help. The main thing has just been testing it, I haven't had much time lately.

The loading mechanism in this branch has a quite a few changes to before, we need test:

  • only config
  • except config
  • Loading this into parent apps directly
  • Addon compatibility in parent apps
  • Specifying different versions of D3 v4.x

@gtb104
Copy link

gtb104 commented Oct 4, 2016

I'll see if I can get mgmt to buy off on me test-driving this.

@gtb104
Copy link

gtb104 commented Oct 7, 2016

After testing I can verify that importing specific d3 modules works. Also, the global d3 property was not available.

I then tried to limit what was packaged up and sent to the browser by setting only config. This doesn't seem to work. My config is:

'ember-cli-d3-shape': {
    only: ['d3-array', 'd3-axis', 'd3-selection', 'd3-shape', 'd3-scale']
}

Unfortunately, everything seems to be removed. See screenshot
image
Perhaps it's the way this app is structured. The above config is for an in-repo addon that's used by another Ember app. The config for the parent app is:

'ember-cli-d3-shape': {
    only: ['d3-format', 'd3-selection', 'd3-scale']
}

I tried adding d3-array to this config, but that didn't make a difference.

@gtb104
Copy link

gtb104 commented Oct 7, 2016

I got only working by defining it on the parent app. I had to add about 8 modules for everything to work, so there was little size savings. Since the d3 library is 400KB, I was hoping that I could cut the size in half.

only: ['d3-array', 'd3-axis', 'd3-collection', 'd3-color', 'd3-dispatch', 'd3-ease', 'd3-format', 'd3-interpolate', 'd3-path', 'd3-selection', 'd3-shape', 'd3-scale', 'd3-time', 'd3-timer', 'd3-time-format', 'd3-transition']

@ivanvanderbyl
Copy link
Owner Author

Ok yeah, if you were just using d3-array it'd save a lot, but typically you need a lot of supporting libs. Good to hear it it works.

@ivanvanderbyl ivanvanderbyl merged commit 5e79fab into ivanvanderbyl:master Oct 16, 2016
@ivanvanderbyl
Copy link
Owner Author

Thanks @spencer516, @mike-north, @gtb104 and @brzpegasus for all the contributions, ideas, and testing that made this PR. It's now merged and I've cut a 0.3.0 release supporting D3 v4.x, I probably need to update the readme a bit more with better documentation.

Please test this out in your apps and report back, after any patches we'll cut a 4.0 release to keep it obviously different to 0.2.0 which is d3 v3.x.

🍻

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.

4 participants