Remove automatic keys

[Olical]

My ideal scenario, an alternate approach to #67 and #73. May not be to everyone's taste.

Also, the scenario where you need to add a key even with just one child can probably be avoided some other way. I will have a think about that one. I just wanted to show that it's pretty easy to just add keys where React tells you to.

Should yield more efficient applications too, right now I would imagine many people using this are seeing far higher CPU usage because they don't understand the key system. Which is technically undocumented!

[Olical]

I'm not saying "THIS IS WHAT IS HAPPENING". This is just my proposal, I wanted to make something to compare to the original PR.

[kitten]

Like I said in #73, I don't think it's a good idea to do this here. We're fixing the problem, but requiring users of this lib to react to the now missing keys.

We already know what the keys are going to be, since we're dealing with a mutable API. So this is imho just half way to what #73 does.

@FunkMonkey

[Olical]

Well, it would be a major version bump and it's good that it requires action. Those that previously got away with bad keys (and they are bad, they have no meaning and are not tied to the element's identity) would be forced to fix them, improving performance and debugging (maybe).

I just never liked the fact that I was doing something behind the users back, sneakily filling in a gap you're meant to do yourself. I'm not sure what you mean by "we already know what the keys are going to be" though.

Is it really that bad to add keys to multiple children, just like you'd have to in regular React anyway? I think not. I think it keeps the faux DOM inline with how React actually works instead of diverging from it.

[Olical]

Added the optimisation such that:

<div>{[child]}</div>

Is now:

<div>{child}</div>

Which does not require a key. Cleaned up that recursion call in toReact at the same time. I do worry about recursion depth there.

[FunkMonkey]

@Olical I do understand your intent and I admit that it is easy to add keys for code that you write yourself. But what about code that you don't write yourself? Even the simplest things like d3.svg.axis will give you React key warnings, as they produce elements like this:

<g transform="translate(0,420)" class="x axis">
  <g transform="translate(0,0)" style="opacity: 1;" class="tick">
    <line x2="0" y2="-420"></line>
    <text x="0" y="3" style="text-anchor: middle;" dy=".71em">2000</text>
  </g>
  <g transform="translate(86.76724137931035,0)" style="opacity: 1;" class="tick">
    <line x2="0" y2="-420"></line>
    <text x="0" y="3" style="text-anchor: middle;" dy=".71em">2001</text>
  </g>
  <g transform="translate(173.29741379310346,0)" style="opacity: 1;" class="tick">
    <line x2="0" y2="-420"></line>
    <text x="0" y="3" style="text-anchor: middle;" dy=".71em">2002</text>
  </g>
  <path d="M0,-420V0H880V-420" class="domain"></path>
</g>

How would we deal with that?

[kitten]

@FunkMonkey Agreed. It's not the same to ask the user for "filling in the gaps" in faux-dom, like React can.

we already know what the keys are going to be

@Olical I mean, that faux-dom Elements are mutable. We are keeping them around and mutating them, eventually calling toReact for every render. Since these elements are not recreated inside the render method, they keep the same instance. Thus we can automatically prevent the diffing problem, that React runs into. keys in React are there for solving this exact problem: To give the diff-ing algorithm a hint of which new ReactElement is replacing which old one, so that the changes can be applied accordingly. We know which Element maps to which ReactElement, even with the changes. Thus we can keep around an ID for the FauxElement using which we can indicate the ReactElements to which DOMElement they belong.

[Olical]

Ah, there's an interesting difference in our use case then. I'm essentially
allergic to state, so I try to minimise it wherever possible (which is why
I wrote this! :D). So I create a new DOM on every render whenever I use
this. See:
https://github.com/QubitProducts/d3-react-sparkline/blob/master/src/d3-react-sparkline.js

I wouldn't recommend keeping the element around outside of the render call,
if you want to do that you may as well start stepping out of React and
letting D3 have full control of that element anyway. The point of the faux
DOM was that you could use it as part of a pure function.

On 21 September 2016 at 14:10, Phil Pluckthun notifications@github.com
wrote:

@FunkMonkey https://github.com/FunkMonkey Agreed. It's not the same to
ask the user for "filling in the gaps" in faux-dom, like React can.

we already know what the keys are going to be

@Olical https://github.com/Olical I mean, that faux-dom Elements are
mutable. We are keeping them around and mutating them, eventually calling
toReact for every render. Since these elements are not recreated inside
the render method, they keep the same instance. Thus we can automatically
prevent the diffing problem, that React runs into. keys in React are
there for solving this exact problem: To give the diff-ing algorithm a hint
of which new ReactElement is replacing which old one, so that the
changes can be applied accordingly. We know which Element maps to which
ReactElement, even with the changes. Thus we can keep around an ID for the
FauxElement using which we can indicate the ReactElements to which
DOMElement they belong.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#74 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATPXarVtKoqJ-XBHyxBfc3uafzJ0mG_ks5qsSy-gaJpZM4KBhOt
.

[FunkMonkey]

@Olical What about your animate-d3-with-mixin example? It will throw key warnings due to d3.svg.axis

It is not really about keeping the the faux elements around or recreating them each time in render, but more about being able to use 3rd party code with d3.svg.axis being a very basic example that will fail.

I personally like react-faux-dom because I can mix creating SVG elements using React with extending these elements using d3, f. ex.:

function render() {
  const fauxXAxis= Faux.createElement( 'g' );
  d3.select( fauxXAxis )
    .call( d3.svg.axis() );  // no control over keys of ticks

  const fauxYAxis= Faux.createElement( 'g' );
  d3.select( fauxYAxis )
    .call( d3.svg.axis() );

  const circles = [ 100, 200 ].map( ( val, i ) => <circle key={i} r="10" cx={val} cy={val} />
  const circleGroup = <g key="circles">{circles}</g>

  return (
    <svg height="300" width="300">
      { [ fauxXaxis.toReact( 'x' ), fauxYaxis.toReact( 'y' ), circleGroup ] }
    </svg>
  );
}

This simplified example is the main reason I use react-faux-dom and I really like that it makes things like this possible ;)

[kitten]

@Olical My PR also doesn't abolish creating faux-dom elements on the fly. It "falls back" to the element order in that case.

It also doesn't prevent the user from using attr('key', x)

[Olical]

Ah, I see what you mean with those deep structures now. I'll go over your PR again then, there's a couple of changes I made here that I would like to see in yours, only small things. Maybe I'll merge yours and just do it myself, namely the function I refactored out for the recursion.

[Olical]

Closing this because I now realise automatic keys are required. I don't think global automatic keys are the way though, so a middle ground would be awesome. The getChild refactor will be useful though.