curran/d3-component

v3 API Changes

curran opened this issue · 6 comments

Summary of API changes between v2 and v3:

  • No longer using this in create(), render(), destroy()
  • Passing a selection as the first argument to create(), render(), destroy()
  • No longer passing nodes into create(), render(), destroy()
  • No longer passing i into create(), render(), destroy()

Before: .render(function (d, i, nodes){ var selection = d3.select(this);

After: .render(function (selection, d){

One driving force is ES6 considerations. Using d3-component is awkward with ES6, because the API relies heavily on use of non-lexically-bound this. For the next major version of d3-component, I'd like to make the API more ES6-friendly.

The decision for the original API was intentional, to mirror the API of D3's selection.each. However, it's unfortunate because we can't use ES6 arrow functions.

ES5:

var paragraph = d3.component("p")
  .render(function (d){
    d3.select(this).text(d);
  }),

ES6 (ideal):

const paragraph = d3.component("p")
  .render((d) => d3.select(this).text(d)); // Breaks due to lexically bound "this"

Maybe we could pass in a selection:

ES6 (idea):

const paragraph = d3.component("p")
  .render((selection, d) => selection.text(d)); // Would work fine.

/cc @micahstubbs

@curran in practice, when I need to access this in the es5 way from an es2015+ codebase, I just say

function(d) {
 d3.select(this).append('circle')
    // set attributes and so on...
}

the rest of the time, I use an arrow function. I arrived at this place by using lebab to automatically upgrade es5 code to es2015. its arrow transform preserves functions that contain this as is. so, when I use lebab, I don't really worry about this issue. it automatically uses arrow functions if it safely can.

@micahstubbs Do you think this would be a worthwhile API change for d3-component? It would allow for use of arrow functions for lifecycle hooks, which is currently not possible.

@curran don't have strong feelings about this. my intuition is that it would it remove a special case a user of d3-component has to remember, and that would be positive ☺

@curran I personally already have a strong habit of checking for this lexical this special case, hence the lack of strong feelings 😉

that said, it's important to think about the new user experience

Yeah totally. It would one fewer WTF/minute.

image

Dealing with this always feels icky to me, because I never quite feel like I can trust what its value will be. My gut feeling is that this change would clean up the API nicely.

I'm thinking to change the API from this (v2.X API):

  var myComponent = d3.component("div", "some-class")
    .create(function (d, i, nodes){ // Invoked for entering component instances.
      d3.select(this)
          .style("font-size", "0px")
        .transition()
          .style("font-size", "80px");
    })
    .render(function (d, i, nodes){ // Invoked for entering AND updating instances.
      d3.select(this).text(d);
    })
    .destroy(function (d, i, nodes){ // Invoked for exiting component instances.
      return d3.select(this) // You can return a transition here to delay node removal.
        .transition()
          .style("font-size", "0px");
    });

to this (v3.X API):

  var myComponent = d3.component("div", "some-class")
    .create((selection, d, i) => { // Invoked for entering component instances.
      selection
          .style("font-size", "0px")
        .transition()
          .style("font-size", "80px");
    })
    .render((selection, d, i) => { // Invoked for entering AND updating instances.
      selection.text(d);
    })
    .destroy((selection, d, i) => { // Invoked for exiting component instances.
      return selection // You can return a transition here to delay node removal.
        .transition()
          .style("font-size", "0px");
    });

Note that the third argument nodes is also removed in this API proposal, I haven't found it useful once yet. I just added it originally as kind of a "cargo cult" thing because it's there in D3's selection.each, probably inspired by the third argument of JavaScript's forEach.

Philosophically speaking, each component should be responsible for managing things that only depend on the datum (d). Depending on the index i even seems like "too much information" in a way, because the component should not be doing anything that depends on the ordering of the data array.

In hindsight, I modeled the API after the wrong thing, selection.each(). What the API should have been modeled after is selection.call(). A component is more like a function that gets passed into selection.call() than a function that gets passed into selection.each().

Therefore, I think the API should be:

  var myComponent = d3.component("div", "some-class")
    .create((selection, d) => { // Invoked for entering component instances.
      selection
          .style("font-size", "0px")
        .transition()
          .style("font-size", "80px");
    })
    .render((selection, d) => { // Invoked for entering AND updating instances.
      selection.text(d);
    })
    .destroy((selection, d) => { // Invoked for exiting component instances.
      return selection // You can return a transition here to delay node removal.
        .transition()
          .style("font-size", "0px");
    });