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

enable configuration of a different window object #1250

Closed
wants to merge 9 commits into from

Conversation

sammyt
Copy link

@sammyt sammyt commented May 16, 2013

Presently when running in node.js, d3 creates it's own window object to operate on. While initially convenient this makes it quite difficult to have independent flows of execution all using d3.

This change enables the creation of new d3 instances which are bound to provided windows.

My driving motivation is that I want to use d3 to build my views on both the client and the server. On the server I need each request to have an isolated environment.

Here is an example of using the proposed d3.env function with jsdom

jsdom.env(
  "<html><head/><body><div class='wibble'></div></html>"
, function(err, window){
    var d3 = require('d3').env(window)
    d3.select('.wibble')
      .classed('wibble', false)
      .classed('wobble', true)
    console.log(window.document.innerHTML)
    // <html><head></head><body><div class="wobble"></div></body></html>
  }
)

As well as allowing for isolated windows this enables the use of alternative DOM implementations. As an example for my own purposes I believe domino is more appropriate.

var domino = require('domino')
  , win = domino.createWindow('<head></head><body><div id="foo"></div></body>')

var d3 = require('d3').env(win)
d3.select('#foo').classed('dimdom', true)
console.log(win.document.innerHTML)
// <html><head></head><body><div id="foo" class="dimdom"></div></body></html>

Grateful for you thoughts
sammy

@mbostock
Copy link
Member

This adds an extra layer of closures on all of d3, so has a performance cost in the common case where only the current window is needed. I see it as being valuable to support this use case, but I’d greatly prefer to do it in a way that doesn’t cost other use cases (particularly the main one, where D3 is loaded in a browser).

For example, perhaps there’s something we can do with d3_document and d3_window (in document.js).

@sammyt
Copy link
Author

sammyt commented May 16, 2013

That makes total sense, I'll have a think about alternative solutions. Thanks for getting back to me so quickly.

@sammyt
Copy link
Author

sammyt commented May 16, 2013

perhaps this change would make more sense?

In the above commit I've added a new build specific to node.js. It adds a little build time overhead but avoids the performance penalty for the common case.

@mbostock
Copy link
Member

Eh, that seems complicated (and duplicates src/d3.js), sorry. There’s already a node-specific “build” of D3, in the sense of the index.js that is only used in a Node (require) environment. Also, you might take a look at test/load.js, which uses SMASH to load a separate minimal copy of D3 for each test. That might not be the ideal API for your use case, but I mention it to point out there are already ways of accomplishing what you desire.

@sammyt
Copy link
Author

sammyt commented May 16, 2013

no worries, I'm always happy to go back to the drawing board. The load.js approach is interesting, though it would seem to duplicate a lot of what the build process already does to get a fully fledged d3 out of it.

I'll give it more thought.

@mbostock
Copy link
Member

Right, that’s because it’s loading a minimal instance of D3 to verify that all the required import statements are there.

@nicko
Copy link

nicko commented May 17, 2013

Hi could you use the arguments.callee property of the constructor function and assign it to the var d3 object created on line 2? For example:

d3 = function(win) {
  var d3 = {
    version: "3.1.7",
    env: arguments.callee
  };
  if (!Date.now) Date.now = function() {
    return +new Date();
  };
  var d3_window = win || window, d3_document = d3_window.document;

@sammyt
Copy link
Author

sammyt commented May 17, 2013

That is a really nice solution Nick!

@sammyt
Copy link
Author

sammyt commented May 17, 2013

Would need to remember to pass in the window object when d3 self invokes.

// src/end.js
 return d3;
})(window);

names the self invoking function that creates d3 so a reference to it
can be stored in d3.env
@sammyt
Copy link
Author

sammyt commented May 18, 2013

the same thing could be achieved with a named function expression. I've put together this branch to demonstrate.

The NFE approach (or arguments.callee) provides everything the original pull request does, but without the overhead of the function wrapper.

@sammyt
Copy link
Author

sammyt commented May 18, 2013

updated this pull request to reflect the NFE approach.

@mbostock
Copy link
Member

This looks promising. What about if the name of the argument was d3_window rather than win, and you changed src/end.js to pass in window?

@mbostock
Copy link
Member

(Also, src/start.js is a generated file because it contains the version number, so you’ll need to remake your edits in bin/start.)

@sammyt
Copy link
Author

sammyt commented May 18, 2013

I've updated the bin/start script to generate the src/start.js (hadn't spotted that).

I tried to pass in the window object through src/end.js but it caused the tests to fail. I'm guessing a little here but I think it might be because some of the tests run within a sandbox that doesn't contains a document property. I've added a conditional check to see if d3_window is defined and fallen back to window instead. Is that OK? I could try and fix the tests that break when adding window to end.js if you prefer? :)

@mbostock
Copy link
Member

Right, the issue with the tests is that large parts of D3 don’t depend on having a window or document available. If you change the wrapper function to take a window as its argument, then you now require all tests to have a sandbox with a defined window, whereas previously that only applied to a subset of tests.

Of course, to compile that symbol needs to be defined, but it can still be null. I pushed my edits to an environment branch.

@ZJONSSON
Copy link
Contributor

If I understand this correctly there would be a new complete d3 instance for each additional window. Would it not be better to keep track of local window in the selection prototype, as window is only used in selections?

@mbostock
Copy link
Member

Well, in a sense that’s already possible because you can d3.select(document.documentElement).select("foo"). But besides d3.select and d3.selectAll, there are a few other places that depend on a global window or document, such as d3.mouse and d3.behavior. Although I think it would be possible even there to use element.ownerDocument.defaultView to go from the element to its containing window.

@sammyt
Copy link
Author

sammyt commented May 18, 2013

Splendid stuff, I see what the test harness is doing now. Thank you.

@gabrielmontagne
Copy link

This is very useful.

I'm also using D3 with domino server side, to not only construct HTML documents but also to query and process long and deeply nested XML documents; D3 is very useful in this context.

Wouldn't it be better if there were no default document/window objects on D3 when running server side? Client side, of course, there's no reason not to have them, but server side the use cases are more diverse and the explicit configuration of the document might not be felt as a burden.

At work we don't need jsdom at all but still we run into problems with it because jsdom is a D3 dependency but it won't easily build on our windows machines.

@sammyt
Copy link
Author

sammyt commented May 21, 2013

Yeah I'm also in the position where the inclusion of jsdom as a dependency, and the creation of the default window is unnecessary.

My aim with this pull request was to add the environments concept without breaking anything that currently depends on that default window.

query and process long and deeply nested XML documents; D3 is very useful in this context

very cool.

@mbostock
Copy link
Member

mbostock commented Feb 7, 2015

Fixed in #2225; D3 no longer assumes a global window or document. So, if there’s no global window or document, you must call d3.select(node).select(string) to first specify the root node of a selection rather than calling d3.select(string) which tries to use the global document.documentElement.

@mbostock mbostock closed this Feb 7, 2015
@mbostock mbostock modified the milestones: 3.5.x, 3.x Feb 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants