d4 icon indicating copy to clipboard operation
d4 copied to clipboard

Refactor as node module

Open nsonnad opened this issue 9 years ago • 7 comments

Hello again! Implementation-wise this is not too different from #14, but I'm starting to realize that the implications reach far beyond browserify. One amazing and under-used feature of d3 is its ability to run on node, which means you can pre-render charts on the server. Would be excellent if d4 could do the same.

I don't think there's anything major blocking it except code structure and the dependency on a browser global, as discussed in the other issue, as d3 is fully node-compatible. Thoughts @heavysixer ?

nsonnad avatar Jun 05 '15 13:06 nsonnad

Hey @nsonnad as far as I understand your point I think you are entirely correct. It may be just reconfiguring the way d4 exposes itself as a variable. I assume we can follow whatever steps d3 does since it obviously works within node.

heavysixer avatar Jun 05 '15 14:06 heavysixer

I guess we'll want to support all of: UMD, bower, CommonJS (ie node), webpack. It looks like in-progress d3 version 4.0 is using ES6 and a custom version of bundler, while d3 3.x uses Makefile targets. There is also browserify's standalone build option, which might be the best option.

nsonnad avatar Jun 05 '15 21:06 nsonnad

+1 for a CommonJS compatible version

mhkeller avatar Aug 14 '15 19:08 mhkeller

A very quick attempt at this and found some issues to work out. I haven't been able to run the tests since that rebuilds the library and I've simply made changes to the compiled d4.js file.

  • I got rid of all intermediate anonymous functions and made one large one
  • It relies on a global d3 reference, so in its environment check, it should load var d3 = require('d3') if it's in CommonJs etc.
  • For some reason I get the following error on tspan.
  if (tspan.node().getComputedTextLength() > width - Math.abs(x)) {
                         ^
TypeError: tspan.node(...).getComputedTextLength is not a function

Logging tspan gives

 [ { _events: {},
      _childNodesList: null,
      _ownerDocument: [Object],
      _childrenList: null,
      _version: 1,
      _memoizedQueries: {},
      _readonly: false,
      _namespaceURI: 'http://www.w3.org/2000/svg',
      _prefix: null,
      _localName: 'tspan',
      _attributes: [Object],
      _settingCssText: false,
      _style: [Object],
      _attached: true,
      _attachedToDocument: true,
      __data__: 700 },
    parentNode: { _events: {},
      _childNodesList: null,
      _ownerDocument: [Object],
      _childrenList: null,
      _version: 2,
      _memoizedQueries: {},
      _readonly: false,
      _namespaceURI: 'http://www.w3.org/1999/xhtml',
      _prefix: null,
      _localName: 'html',
      _attributes: {},
      _settingCssText: false,
      _style: [Object],
      _attached: true,
      _attachedToDocument: true } ] ]

Commenting out that if statement seems to get the library working. I'll try and find time to investigate more and get the tests working to see about any unintended consequences. If someone else wants to jump on too that'd be great.

mhkeller avatar Aug 14 '15 20:08 mhkeller

Thanks for the progress @mhkeller. Let me see if I can help suss things out. I think that @nsonnad was working on this too.

heavysixer avatar Aug 14 '15 20:08 heavysixer

@mhkeller is your work in a fork i can see?

heavysixer avatar Aug 17 '15 13:08 heavysixer

Here you go. The diff is ugly since the version I had pulled from npm was 8.10 but I forked a more recent version. I was gonna redo but I couldn't find the exact tag in the GitHub releases so figured the diff would be messy in any event.

To help, I've added inline comments documenting the bulleted changes described above: https://github.com/mhkeller/d4/commit/d16c91c488e8b93af8df90b3fb0f26685a90de10

mhkeller avatar Aug 17 '15 14:08 mhkeller