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

Fix log axis and update builds. #40

Merged
merged 3 commits into from
May 9, 2017
Merged

Fix log axis and update builds. #40

merged 3 commits into from
May 9, 2017

Conversation

samussiah
Copy link
Contributor

@jwildfire , @nbryant , babel wasn't cooperating with Node 6 (example issue here) in the es5 build so I've made a few additions to the webCharts config:

  • added rollup-plugin-babel and babel-preset-es2015 to devDependencies in package.json
  • added rollup-config.js and .babelrc for the es5 build

All this just to fix the super minor log axis issue.

Closes #38
Closes #33

Nathan, please let me know if anything looks off. Instead of concatenating the UMD files open and close to the babel'd output, I'm just specifying the umd format in rollup.config.js. Works in Chrome and IE but I don't know how to test in the Node environment.

@nbryant
Copy link
Contributor

nbryant commented Mar 22, 2017

@samussiah I'm sure I've forgotten my original reasoning behind the many different builds, but it seems like we could actually just have the one build that uses the UMD format that you've set up. The extra .amd.js, .common.js, etc. builds in lib will then just be redundant and unnecessary - the UMD format should work in any environment. I think I set this up when rollup was very new, and it might not have had UMD as an option for the format parameter, hence the funky file concatenation. Or I was oblivious ¯_(ツ)_/¯

One tweak you'll need to make is to change the rollup config to not exclude d3 as a global. Rather, we do want it specified as a dependency. Notice the difference between the block at the top of your build/webcharts.js and the current build/webcharts.js.

To test in Node, you can basically just run node from the command line, then do var webcharts = require('/path/to/webcharts/') and confirm that it pulls in the webcharts object as expected.

@samussiah
Copy link
Contributor Author

@nbryant I'm pulling my hair out. I can't get rollup to pass d3 as an argument in the build without it being renamed d3$1. Would you check out the updated rollup.config.js and index.js? The only thing I can think of is that d3 needs to be imported into each .js file which uses it rather than into index.js.

On another note I remove \lib and associated scripts in package.json.

@samussiah
Copy link
Contributor Author

Went with a hot fix which is probably why we had lib/open.js and lib/close.js in the first place:

"es5": "rollup --config && sed -i 's/d3$1/d3/' build/webcharts.js",

rollup handles the UMD wrapper and treats d3 as a global as it should but doesn't pass d3 to webcharts, instead renaming it d3$1:

(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory(require('d3')) :
	typeof define === 'function' && define.amd ? define(['d3'], factory) :
	(global.webCharts = factory(global.d3));
}(this, (function (d3$1) { 'use strict';

The hot fix replaces d3$1 with d3. This wrapper format matches the current wrapper format:

(function (root, factory) {  
	if(typeof define === "function" && define.amd) {    
		define(["d3"], factory);  
	} 
	else if(typeof module === "object" && module.exports) {    
		module.exports = factory(require("d3"));  
	} 
	else {    
		root.webCharts = factory(root.d3);  }
	}(this, function(d3){

so I think we're good to go.

@nbryant
Copy link
Contributor

nbryant commented Mar 27, 2017

Sorry, I've been really busy and slow to respond. The d3$1 issue rings a bell, but, looking at that code, I don't see why it would be a problem. The global d3 is fair game to be renamed, since it is just aliased as d3$1 when passed as an argument to the factory function. As long as all the subsequent code in the factory function references d3$1, there should be no problem, right? Or were you getting some errors when trying this out?

@jwildfire jwildfire mentioned this pull request May 9, 2017
@jwildfire
Copy link
Contributor

I just tested the current build a bit and (besides the one missing devDependency) everything built as expected and charts seem to render just fine.

Just to clarify, the hotfix mentioned above is not included in this PR, and is probably not needed in the future (as noted by @nbryant above). (I do agree that the way aliasing is handled in rollup is a bit strange though)

@jwildfire jwildfire changed the base branch from master to v1.7.2-dev May 9, 2017 22:41
@jwildfire jwildfire merged commit 5154c30 into v1.7.2-dev May 9, 2017
@samussiah samussiah deleted the log-axis-fix branch May 10, 2017 17:50
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.

3 participants