Add type parameter to `local` in `d3-selection` module

[gustavderdrache]

Since d3.local() creates an object that acts kind of like a Map<Element, T>, it seems wisest to add a type parameter in order for the compiler to track the type of data stored.

This PR also adds a few conveniences around the use of Selection.property, hinted at by the documentation:

If you are just setting a single variable, consider using selection.property:

selection.property(foo, function(d) { return d.value; });

I played around with it in the console and it does indeed work well:

> let local = d3.local(), p = d3.select('p');
> local.set(p.node(), [1, 2, 3]);
> local.get(p.node());
[1, 2, 3]
> p.property(local, [4, 5, 6]);
> p.property(local);
[4, 5, 6]
> local.get(p.node());
[4, 5, 6]

I've added to the tests to cover this as well as Local.remove, which was missing from the original test suite.

[tomwanzek]

Thanks for the PR! I will get back to it shortly. I am just going to wrap up the merges related to d3-geo. I have started preparing the follow-up PR for DefinitelyTyped/types-2.0. If all goes well, I get it all wrapped in today. I will cc you on DT as usual.

[tomwanzek]

Great catch, I did not see the use with Selection.property(...) buried in the section on Local, when I went through the API doc.

I guess, the only two things I would suggest are as follows:

(1) Since you rightly suggest templating Local, I think we might as well go all the way, i.p. since both examples in the API doc hint at use cases on the Selection interface (each(...) and property(...)). So it could become Local<GElement extends Element, Datum> or Local<GElement extends BaseType, Datum>. The GElement would template the node related items of get, set and remove. This could go hand-in-hand with three Local-variable factory signatures:

export function local(): Local<Element, any>;
export function local<T>(): Local<Element, T>;
export function local<GElement extends Element, T>: Local<GElement , T>;

I have not fully thought out all the pros/cons of using Element or BaseType as a constraint yet. The signatures you added to property(...), would then read property<T>(local: Local<GElement, T>, ... )

(2) In a follow-up it probably would be great to add this use pattern more prominently to the D3 API for property with respect to the name argument. I had a couple of similar things, I will have to get around to cycling back to the D3 API docs.

Over to you :smile:

[gustavderdrache]

Actually, due to the way d3.local works (it walks up the node tree until it finds a set value), I don't think parameterizing by the element type will do what you'd expect:

> let local = d3.local();
> local.set(document.body, "foo");
> local.get(document.body.firstChild);
"foo"

But having played with it some more, I need to update the types so that it returns T | undefined since the lookup can fail.

As regards item 2, I remember there being a proposal for basically a T[prop] type that would return the value of the property -- since it would greatly help idioms like _.property("foo"). At least, I think that's what you were hinting at.

[gustavderdrache]

I've made a few changes, mostly around documentation comments to help clarify the property<T> overloads, as well as some basic comments to the Local<T> interface.

The more useful commit is 0df5105 which changes the return value to T | undefined since lookups can fail.

[tomwanzek]

Thanks @gustavderdrache . As for my comment above:

(2) In a follow-up it probably would be great to add this use pattern more prominently to the D3 API for property with respect to the name argument. I had a couple of similar things, I will have to get around to cycling back to the D3 API docs.

All I was getting at, is to look into updating the actual D3 API for selection.property(...) to clarify that name can be a string representing a property name, or a Local as identified by you.

One thing, I did not test out wrt to the crawling up of the DOM tree as noted in your example above is this. While

local.get(document.body.firstChild);

works as expected. If one, however, created a selection for document.body.firstChild, then I think:

firstChildSelection.property(local); // will not return "foo"

If I recall correctly, internally property simply coerces local to string which means it uses the internally generated local variable id, e.g. @2, as the property name. In turn, it does not crawl up the DOM tree.

[tomwanzek]

PS. I am going to make a quick sweep across all the modules to check for any version changes compared to the latest D3. I think it will only be version number changes in the header comments. Than I will bundle the changes from this PR with any resulting edits and do a combined PR to DefinitelyTyped.

[gustavderdrache]

One thing, I did not test out wrt to the crawling up of the DOM tree as noted in your example above is this. While

local.get(document.body.firstChild);

works as expected. If one, however, created a selection for document.body.firstChild , then I think:

firstChildSelection.property(local); // will not return "foo"

If I recall correctly, internally property simply coerces local to string which means it uses the internally generated local variable id, e.g. @2 , as the property name. In turn, it does not crawl up the DOM tree.

You are correct about that. I'm hoping that the comment I left explained it sufficiently. It's not totally useless though -- it makes for a nice shorthand when you know the local has been set on the node directly.