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

Refactor as node module #39

Open
nsonnad opened this issue Jun 5, 2015 · 7 comments
Open

Refactor as node module #39

nsonnad opened this issue Jun 5, 2015 · 7 comments

Comments

@nsonnad
Copy link
Contributor

nsonnad commented Jun 5, 2015

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 ?

@heavysixer
Copy link
Owner

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.

@nsonnad
Copy link
Contributor Author

nsonnad commented Jun 5, 2015

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.

@mhkeller
Copy link

+1 for a CommonJS compatible version

@mhkeller
Copy link

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.

@heavysixer
Copy link
Owner

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
Copy link
Owner

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

@mhkeller
Copy link

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: mhkeller@d16c91c

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

No branches or pull requests

3 participants