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

v3 API Changes #70

Closed
curran opened this issue Mar 20, 2017 · 6 comments
Closed

v3 API Changes #70

curran opened this issue Mar 20, 2017 · 6 comments

Comments

@curran
Copy link
Owner

curran commented Mar 20, 2017

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

@micahstubbs
Copy link
Collaborator

micahstubbs commented Mar 20, 2017

@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.

@curran
Copy link
Owner Author

curran commented Mar 24, 2017

@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.

@micahstubbs
Copy link
Collaborator

@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 ☺

@micahstubbs
Copy link
Collaborator

@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

@curran
Copy link
Owner Author

curran commented Mar 24, 2017

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.

@curran curran changed the title ES6 Considerations v3 API Changes Mar 24, 2017
curran added a commit that referenced this issue Mar 24, 2017
@curran curran mentioned this issue Mar 24, 2017
4 tasks
@curran
Copy link
Owner Author

curran commented Mar 25, 2017

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");
    });

@curran curran mentioned this issue Mar 25, 2017
16 tasks
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

2 participants