couchand / d3-redux

:bar_chart::floppy_disk: Idiomatic D3.js bindings for Redux.
MIT License
9 stars 0 forks source link

Pass state via connect function #7

Open danielnaab opened 6 years ago

danielnaab commented 6 years ago

While trying to structure a growing D3-rendered app, I was very pleased to encounter your work with this library. I'm integrating it now and am happy with how it's helping to streamline things.

However, I'm struggling a bit with certain usages of state - dealing with situations where the structure of the DOM, rather than just the data binding, is triggered off application state.

For instance, I'm creating scales and axes using the D3 modules. Because these are derived from application state, a reselect selector is used to create them. I then need to call() these functions. As a simple example:

function xAxisComponent(elem, store) {
    const xAxis = xAxisSelector(store.getState());
    elem.call(xAxis);
}

elem.call(connect(xAxisComponent), store);

Would it make sense to modify the connect function to pass (node, state) to deal with this kind of thing?

danielnaab commented 6 years ago

Here's a bit more thought out alternative of what I'd like to see. I can provide a PR if you'd accept it.

function xAxisComponent(elem, xAxis) {
    elem.call(xAxis);
}

elem.call(connect(xAxisComponent, xAxisSelector));

Connect can then optimize the call to xAxisComponent, skipping the call when the selector returns a different object from the last invocation.

In addition to providing access to the state in a more general manner, it means we can write D3 rendering functions that don't need to know about Redux (unless dispatching actions) - which makes them easier to write unit tests against.

couchand commented 6 years ago

Hi @danielnaab, sorry about the delay replying. I spent a while chewing over your use case when you first filed this issue, then it slipped my mind before I replied.

This is a great question. I had already been thinking about how this version of connect only does half of the job of connect from react-redux: the render-on-change part. Your suggestion is natural, to add the selector and action binding part, and the proposed API makes sense in many ways.

As background, let me quickly talk about two of the design principles I've been trying to follow here. First, I'd like to match conventional d3 patterns as much as possible, so that the usage is obvious to someone familiar with d3. I'd also like the API and implementation to be as simple as possible, also with the goal of making usage approachable.

I didn't try to make the connect here do everything that connect from react-redux does, because the prop passing model of react doesn't precisely map to the data binding model of d3. I had hoped the dispatch and fromState methods would be sufficient for idiomatic d3 code.

So with that said, if we could come up with a snippet of code to achieve your goal just by using the current API, that would be ideal. If it's sufficiently non-obvious perhaps it could be added here as a helper method.

Here's my proposal: another little wrapper function. Let's work out what it would need to look like. In the raw form, we want to bind the result of calling xAxisSelector on the state to the selection's data, and then call the axis on each of the items:

function xAxisComponent(selection) {
    selection
        .data(fromState(xAxisSelector))
        .each(function () {
            const item = d3.select(this);
            const xAxis = item.datum();
            item.call(xAxis);
        });
}

as an aside, if we aren't concerned about selections of multiple elements, or if we expect the xAxisSelector to return the same value for each, we can extract the contents of the each, like so:

function xAxisComponentSingle(selection) {
    selection.data(fromState(xAxisSelector));
    const xAxis = selection.datum();
    selection.call(xAxis);
}

or, without the data-binding:

function xAxisComponentSingle(selection) {
    const xAxis = fromState(xAxisSelector).call(selection);
    selection.call(xAxis);
}

Though, admittedly, that looks awfully hacky, though I do like that it doesn't arbitrarily override whatever data binding you may already have, just to make calling fromState nicer.


So after all this brainstorming, I still think the answer is another little higher-order component, rather than changes to connect. In light of the last point above, I think the best way to write it would be:

function componentFromState(componentSelector) {
    return function wrappedComponent(selection) {
        selection.each(function () {
            const item = d3.select(this);
            const component = fromState(componentSelector).call(item);
            item.call(component);
        });
    }
}

Which is definitely non-obvious. And you'd use it like:

elem.call(connect(componentFromState(xAxisSelector)));

Can you give this a try and see if it works as expected? What do you think about the UX of adding an additional function versus changes to connect itself? Do you envision a scenario where the connected component would be more involved than just a wrapper for the selected component?

danielnaab commented 6 years ago

Hi @couchand thanks for your detailed response! componentFromState looks like a nice abstraction - I'll consider your code a bit more this coming week. In the time after this original ticket was created, we ended up copying your work with d3-redux and modifying it a bit, most notably with the addition of a link function that is similar to what componentFromState does, but doesn't re-call the component when the selector's object is the same as the last invocation. It was dependent on a couple changes elsewhere, but the function itself looks like this:

function link(func, selector) {
    let currentOptions = null;
    let context = null;
    return connect(function (selection, state) {
        let nextOptions = selector(state);
        if (currentOptions !== nextOptions) {
            currentOptions = nextOptions;
            context = func.call(null, selection, currentOptions, context);
        }
        return context;
    });
}

Here's the code for that: https://github.com/usgs/waterdataui/blob/master/assets/src/scripts/lib/redux.js#L69-L86 The context parameter facilitates maintaining some state from one invocation of the function to the next.

Perhaps you could give some suggestions on if you consider this pattern consistent with D3. WRT the code itself, I put all the d3-redux functions in the same CommonJS module for simplicity, but would definitely like to merge any changes into your codebase and/or modify things on our end if we could work out the best abstractions.

You can see how the link function is being used extensively in our application, new pages for Water Data for the Nation, by searching for it in the codebase. The principle entry-point for the D3 rendering code is here: https://github.com/usgs/waterdataui/blob/master/assets/src/scripts/components/hydrograph/index.js#L437

So far, there are many uses of the link function throughout the application and it seems to be a pretty useful abstraction. Like useful ones sometimes are, it does require some knowledge of how it works under the hood to avoid gotchas, and it would be nice to eliminate those if possible.

We are using D3 as the general purpose view layer for this application, not just for data binding. I personally found D3's data binding to be restrictive at times - and easier to pass data into the rendering functions directly, and selectively use data binding when dealing with sets of data. Perhaps that could be fine tuned in some way.

We're "soft-launching" the work, but here are a few examples pages we've got live already:

https://waterdata.usgs.gov/monitoring-location/05370000/ https://waterdata.usgs.gov/monitoring-location/07144100/ https://waterdata.usgs.gov/monitoring-location/05413500/ https://waterdata.usgs.gov/monitoring-location/07374000/

There is also an initAndUpdate function (https://github.com/usgs/waterdataui/blob/master/assets/src/scripts/lib/redux.js#L89-L103), which hasn't proven to be super useful yet, but you can find one usage of it here: https://github.com/usgs/waterdataui/blob/48dfd1c9af3f9e6f9f772d637e4561063e54d91a/assets/src/scripts/components/hydrograph/tooltip.js#L201

Thanks for all your great ideas with d3-redux - we've been able to get a lot done in just a few months in large part because of your inspiration. Looking forward to collaborating with you on these abstractions!

@mbucknell @ayan-usgs

couchand commented 6 years ago

Awesome to hear you're actually using this (the idea if not the package itself)! I'd be happy to work together to make sure this project meets your needs.

I agree that only using d3's data binding would be too restrictive for a large application, which is exactly why I wrote this! My hypothesis is that redux complements it very well, and the two together should be sufficient. I'm kind of amazed that there's not already a ton of work on this concept out there, because it seems to me to mesh quite nicely.

Let me state the goal as I'm approaching it, and then see how closely your goal matches up. I'd like to be able to take a reusable chart (as described in Towards Reusable Charts), a redux-store provided d3-selection, and a reselect-selector, and have the chart rerendered when the selected portion of the state changes, getting the selector's result as input.

I would suggest that there are only two universal ways to pass the selector's result in to the chart: the d3 bound data and the additional parameters to the chart (a la d3's selection.call). The latter can't be managed by a simple higher-order function like the current incarnation of connect (though, notably, it would be trivial to do if connect were written as a method on selection as it initially was). So assuming we aren't monkey-patching the prototype of selection (which I think is a reasonable goal as long as it doesn't make things too difficult/annoying), that leaves the bound data as the only option.

As an aside, I think it's okay to use bound data for this. Getting the data back out is even easier than my first code example above suggests -- I had forgotten that each passes the datum as the first parameter. That means passing through the bound data does require a bit of boilerplate, but it happens to be precisely the boilerplate suggested by Mike Bostock in Towards Reusable Charts. The only reason using bound data isn't perfect is that we might have other bound data inherited from the parent that we want to leapfrog over the connect. That seems awfully marginal, but is a good reason to have a vanilla connect that doesn't do data binding.

So let's assume it's okay to have the selector's results passed in as the selection's data (also, wow these terms are hard to keep straight :smiley:). We'll still do mostly the same stuff:

d3.select(".foo")
    .provide(store)
  .select(".bar")
    .call(connect( ... ));

And the question is just what to put in for the .... Well, we want a chart and we want a selector, so how about:

    .call(connect(something(chart, selector)));

Which we can implement along the lines of:

function something(chart, selector) {
    return function wrappedChart(selection) {
        selection.each(function(previousDatum) {
            const item = d3.select(this);
            const datum = fromState(selector).call(item);
            if (datum !== previousDatum) {
                item.datum(datum).call(chart);
            }
        });
    };
}

Now we just need to come up with a good name. Maybe link? :stuck_out_tongue_winking_eye:

Then we can give it a convenience alias as a two-parameter form of connect by adding this line to the definition (just above here):

if (arguments.length > 1) callback = link(callback, arguments[1]);

Two asides. First, your initAndUpdate seems to be in the same spirit as selectAppend from d3-jetpack. You might look at their implementation for ideas.

Second, there's a subtle bug in the changes you've made to connect in the (admittedly rare) case when the several elements in a selection have different parent stores. I'm not sure there's a way to resolve this without making connect change the semantics of the wrapped chart. The subscribe and callback could be done within the each, but then the chart would receive just the one element of the selection rather than the whole group. I don't know if anyone really uses multiple redux stores, but this sort of issue keeps me up at night.


Looking forward to your thoughts on all this. Thanks for starting the conversation.

The reports you're building look really neat. Good luck with the soft launch!

danielnaab commented 6 years ago

Yes, I didn't feel confident the changes would work properly with multiple stores - so not surprised you see an issue there!

Your suggestions seem like they would work for us. I see the benefit from the D3-centric viewpoint of using the data binding. On the other hand, it adds in a DOM lookup on each state change everywhere a subscription is active, and lookups everywhere we need to access the datum inside the component - so it would be a bit less efficient. I don't want to prematurely optimize, but that does give me a bit of a mixed opinion, because we're dealing with an increasingly large Redux store with state transitions on things like mouseovers. The efficiency of reselect evaluation would be partially negated if we need needed to hit the DOM to get the corresponding state each time.

After noting that our connect function breaks the multi-store support, it makes me wonder if we'd be better off not binding the store to a D3 local, and instead passing it in via another manner. If we went that way, we'd be deviating from your approach considerably, especially considering we're currently not making use of fromState at all, as using link and doing the data binding from its data is an alternative that has been working for us.

Do you think the performance considerations are relevant? Thanks, interested in your thoughts!

couchand commented 6 years ago

I wouldn't expect the performance factor to be important in real usage - I think that the hit to the DOM would be vastly outweighed by all the other activity going on during an update. Of course, if it concerns you, I'd recommend that you run some profiles on your actual application to see. I did some simple profiling on a test app and the overhead from using d3 data binding versus a closure to maintain state was negligible, but YMMV.