BinaryMuse / planetary.js

:earth_americas: Awesome interactive globes for the web
http://planetaryjs.com
MIT License
1.6k stars 181 forks source link

d3 and topojson missing from package.json dependencies #1

Closed max-mapper closed 10 years ago

max-mapper commented 10 years ago

I tried to browserify planetary.js, but it complained

Error: Cannot find module 'd3' from 'planetary.js/dist'

because of this line:https://github.com/BinaryMuse/planetary.js/blob/master/dist/planetaryjs.js#L11

I think you just need to npm install --save d3 topojson

BinaryMuse commented 10 years ago

Hey, @maxogden,

I did indeed forget to add those to the package.json--I'll get that fixed. That said, I had a hard time getting Planetary.js to play well with browserify.

With the dependencies installed, creating a bundle from a main.js of

var planetaryjs = require('planetary.js');

with

$ ./node_modules/.bin/browserify -t brfs main.js > bundle.js

results in the following exception when used in the browser:

Uncaught TypeError: Object #<Object> has no method 'readFileSync'

It's down in TopoJSON's main, at

var topojson = module.exports = new Function("topojson", "return " + fs.readFileSync(__dirname + "/topojson.js", "utf8"))();

Do I just fail at brfs or is something funky up with that line of code that brfs can't detect?

max-mapper commented 10 years ago

ah, interesting. this creates a valid bundle for me: browserify -r topojson -t brfs, so maybe browserify transforms don't apply to dependencies?

cc @substack

max-mapper commented 10 years ago

w00t, when I added

  "browserify": {
      "transform": ["brfs"]
  },

to the topojson package.json then I could do

browserify -r planetary.js

and it created a working bundle

I wonder if theres a way to do it without modifying topojson?

BinaryMuse commented 10 years ago

Yes, you're right. Help even says

--transform, -t  Use a transform module on top-level files.

You can ensure that the brfs transform runs globally with

./node_modules/.bin/browserify --g brfs main.js > bundle.js
BinaryMuse commented 10 years ago

So one solution is to document the browserify command necessary to create the bundle. Right now the AMD/CommonJS info is in the FAQ; I'm going to move it into one of the first core documentation sections so it's more visible.

Transforming D3 with brfs does make the build considerably slower (only a few seconds, but it's 3-4x slower), so perhaps a better solution would be to see if Mike would be willing to accept a pull request for the browserify portion of TopoJSON's package.json.

I suppose it's also possible @substack would weigh in with something awesome that we haven't considered.

max-mapper commented 10 years ago

talking w/ substack in IRC now

I have made https://github.com/maxogden/topojson/commit/bb1d7959f689a45ad129de061c7f012a58c4ad6d which solves the problem, but substack thinks topojson can be changed to remove the need for brfs in the first place

BinaryMuse commented 10 years ago

The dependencies in package.json landed in 05313d79f8f531a4a8cd91c2ca9633e4556e3cd6 and the installation instructions have been updated to use the workarounds above with a note that the process is likely to change; if and when TopoJSON is updated, I'll update the dependency to point to that version and change the instructions.