Fix automatic keys and Elements with a single child #73

kitten · 2016-09-08T10:57:22Z

Before keys were only unique on a single level.
Thus when a tree with two separate groups of children
were generated, these children could still have the same
keys.

This fix appends the element index to the previous key.

There is a remaining problem: When the user has multiple
faux doms, these doms can still have overlapping ids.

This can be circumvented however, by the user
passing his custom key prefix:

elA.toReact('first-dom')
elB.toReact('second-dom')

Before keys were only unique on a single level. Thus when a tree with two separate groups of children were generated, these children could still have the same keys. This fix appends the element index to the previous key. There is a remaining problem: When the user has multiple faux doms, these doms can still have overlapping ids. This can be circumvented however, by the user passing his custom key prefix: ```js elA.toReact('first-dom') elB.toReact('second-dom') ```

Since multiple core mixins can have identical keys, this generates an increasing ID.

Olical · 2016-09-08T11:43:59Z

Nice, thanks for the PR. Although it does solve the issue I'm not sure about changing those top level arguments or further complicating the current implementation (even if only by a little!).

I do wonder if a better solution would be to not set a key at all and just let the user handle it. I am beginning to feel that defaulting these keys is assuming too much and potentially harmful. I would actually like React to warn me when I need to provide a key so I can provide something unique and relevant for optimisation.

Just adding random UUIDs actually obscures quite a powerful optimisation strategy within React, potentially causing a huge performance hit.

@FunkMonkey thoughts? Since you raised the original issue.

kitten · 2016-09-08T13:14:00Z

@Olical I think it's not a good idea to require this from the user... It's a good idea to provide this information, since the elements are constants. I'd refer you to this comment here: facebook/react#1342 (comment)

This doesn't complicate things a lot, but I tried to remove the initial key requirement, but due to some ways that you can create the elements with, I can't assume a "base key".

Not providing a key by default is just as bad as duplicating keys imho, because there might be unexpected things happening to these elements:

TL;DR Use unique and constant keys when rendering dynamic children, or expect strange things to happen.

https://coderwall.com/p/jdybeq/the-importance-of-component-keys-in-react-js

An alternative would be, to just assign keys, when there a lists, which covers only one important case, but would work.

Edit: Adding a UUID is just as bad as not adding a key, if it's not constant

FunkMonkey · 2016-09-11T10:29:33Z

@Olical @philpl

I think keys should not be generated by default, but only be user-provided - just as they have to be for normal React. The exception is obviously generating the react elements for the children (see line 284 - more about that later)

Before keys were only unique on a single level.
Thus when a tree with two separate groups of children
were generated, these children could still have the same
keys.

Keys don't have to be globally unique, but only between siblings. Thus having a prefix is not really necessary.

As for line 284: generally it is advised not to use indexes as react keys as keys are about identity to find nodes even if children have been shuffled around (as you can also read in the articles provided by @philpl). Unfortunately I don't see any other way here, as the code that uses react-faux-dom doesn't really care about React's consequences of shuffling children by using functions like appendChild, insertBefore and removeChild.

Though maybe there is one solution: Every Element should have a globally unique _key. In line 284, if an el.props.key is undefined we will pass the unique el._key to el.toReact.

return React.createElement(this.nodeName, props, this.text || this.children.map(function (el, i) {
    if (el instanceof Element) {
      return el.toReact( isUndefined(el.props.key) ? el._key : el.props.key )
    } else {
      return el
    }
}))

This seems a little convoluted at first, but it means that the top-most React element must not have props.key if the user doesn't want it, but its children at the same time have constant ids that can be properly tracked by React even if the non-react-code uses insertBefore and such. There need to be slight changes in toReact too (e.g. using provided key parameter instead of props.key if it is not undefined).

What do you think?

kitten · 2016-09-11T13:21:57Z

@FunkMonkey I would've done that, but due to the way that faux-dom works, you can dynamically create elements, as you need them in render methods. That means that the _key element might look like a unique, new one to React.

However, I believe this is a better solution than letting the user provide a key, whenever this situation occurs, and prevents bugs.

Also, no -- keys do not have to be unique across different trees, but they have to be:

stable,
predictable,
and unique

The easiest way to do this is through these concatenated ones, but now that I think of it, a correct solution that regards all DOM methods like insertBefore and removeChild might then be problematic.

This can be solved by attaching a unique ID, like @FunkMonkey said, maybe even on the parent's level instead of the child's (?), but I believe that this requires deprecating Element-creation inside the render methods, to produce 100% predictable and correct results.

FunkMonkey · 2016-09-11T14:59:48Z

I would've done that, but due to the way that faux-dom works, you can dynamically create elements, as you need them in render methods. That means that the _key element might look like a unique, new one to React.

I am not quite sure I follow. Which render function do you mean? React's? Could you please give me a code example? Thank you! :)

kitten · 2016-09-12T09:56:00Z

@FunkMonkey First example on the readme:

class SomeChart extends React.Component {
  render () {
    // Create your element.
    var el = ReactFauxDOM.createElement('div')

    // Change stuff using actual DOM functions.
    // Even perform CSS selections!
    el.style.setProperty('color', 'red')
    el.setAttribute('class', 'box')

    // Render it to React elements.
    return el.toReact()
  }
}

Since we create an element on each call of render, we can't randomly generate IDs at any time.

Edit: Maybe you're totally right from the start, and we should not even try to optimise this into infinity, and just omit keys wherever possible? This would at least not spark any unexpected problems. (On the other hand, just adding index-keys to array elements makes reordering a hard diff for React)

Olical · 2016-09-12T10:02:22Z

I personally like the idea of doing away with automatic keys (principal of least surprise?) in favour of how react usually works. You don't need them until you reach a list. At which point the user can assign meaningful keys (maybe hashes of their internal values, that's what I do in ClojureScript anyway).

This yields better performance and doesn't do things behind the developers back.

I started writing this before you replied, @philpl :) this is an interesting discussion and I'm glad we're having it. I want everyone's opinions so we can follow the best path.

I don't see what's wrong in your example though, you don't need any keys there for React to work? Actually, you don't need keys anywhere I guess, but React will warn you when it needs it. Generating keys just feels like assigning the index of your loop as the key, it'll suppress the error from React, but won't actually fix that error (React being unable to reuse the DOM properly).

FunkMonkey · 2016-09-12T19:23:14Z

@philpl @Olical

I don't see a need to add a key in this situation either.

You don't need them until you reach a list. At which point the user can assign meaningful keys (maybe hashes of their internal values, that's what I do in ClojureScript anyway).

This is an important point. I think the user should be able to provide a key to toReact, whenever he/she needs to, but can omit it in most cases.

Internally though, as the children of an Element are a list, their toReact should definitely be called with a key. I still propose this one to be a unique identifier though instead of an index as explained earlier. This won't help in @philpl's example, but it seems creating Elements in the render method is an anti-pattern anyway and will also not work with many libraries which store references to the Elements for later modification. That's certainly why the root Element in the d3 example is created in componentDidMount by using connectFauxDOM.

TL;DR: use unique keys (taken from a child Element) in line 284 and only set props.key if a key is provided as an argument to toReact.

What do you say?

Olical · 2016-09-12T19:30:43Z

I don't think the key should be a special case, there's no need. You can just do this.

By special case, I mean an argument to createElement, I don't think key should be treated specially. What happens if React changes the name of it in the next version? Or removes it entirely?

const el = ReactFauxDOM.createElement('div')
el.setAttribute('key', 'foo')
return el.toReact()

I don't even think removing it would require much of a change to the documentation etc. Obviously a major version bump, but that's not a problem.

Also, it's perfectly acceptable to create elements within the render function. That's what I do. The point is you can keep your render pure but use stateful libraries. The connectFauxDOM stuff is for when you want to use something that continues to mutate the DOM after it has been mounted.

I say we remove all key code, let people set it with the normal DOM setter APIs or through D3 etc. Just show an example in the readme. It'll work just how it does with normal react, but instead of <div key=... it'll be div.setAttribute('key', ...).

kitten · 2016-09-12T19:36:31Z

@Olical @FunkMonkey I see, I believe using unique identifiers in case of lists is perfect, if we make creations of Elements inside the render method an anti-pattern -- which makes perfect sense.

I think I'll change this PR to make toReact accept ids instead of indices. These ids should probably live in FauxElement.key and should be extracted by the parent.

Thus elements will only have a key if it's being passed to toReact. Either by the user explicitly or by a parent element.

Not having automatic keys for lists is still something, that we shouldn't just forgo. It's essential for optimisation. These are still (fake) DOM Elements. So I wouldn't require the user to set keys to manually optimise when we can abstract this away. If the user passes a faux element to a library, they might not even have the opportunity to set a key in a clean way.

Edit: we can even defer the generation of the id until it's needed, so that there is never a key when children.length <= 1?

FunkMonkey · 2016-09-12T19:39:19Z

@Olical This makes sense for the top-level API that the user calls, but what about:

return React.createElement(this.nodeName, props, this.text || this.children.map(function (el, i) {
    if (el instanceof Element) {
      return el.toReact(i)
    } else {
      return el
    }
}))

We still need a key there, as we have a list that the user may not be able to add keys to... as I now also see @philpl wrote at the same time... :)

kitten · 2016-09-12T19:41:11Z

@FunkMonkey Exactly. Nailing this down to automatic + toReact(key) would be very easy to use and no extra work for the user.

Olical · 2016-09-12T19:41:13Z

Isn't this a core tenant of react though? If you need a list of something you wrap it and add a key? If adding default keys and the like was recommended, wouldn't React have implemented it that way? I feel like the faux DOM is parallel to the normal React tree, so it should be able to be used in the exact same way. Maybe I'm wrong though :S I'll have to have a think.

kitten · 2016-09-12T19:43:30Z

@Olical Yes, it is. But in React you create elements dynamically. They can't have a unique ID that tells React when they've changed, since they're immutable. React would still need to diff.

By default React just uses the index, like the user does, which is bad when elements are moving.

In this case we have a DOM-like API, where the Elements are mutable, so we actually can have unique identifiers.

FunkMonkey · 2016-09-12T19:45:32Z

@Olical Also if we remove the keys when mapping the children, React will warn us about missing keys due to the reasons @philpl just explained. See also the link he posted a couple of days back: The importance of component keys in React.js

The Element sets its key in this priority: - To `props.key` - To the argument passed as `toReact(id)` When the children of a Faux Element are a list, a unique key is automatically generated. It can be changed by setting `el.key`. This is not the same as `props.key`, since `props.key` is always in use, and `this.key` is not to be accessed by the user normally.

kitten · 2016-09-12T20:16:00Z

@Olical @FunkMonkey So I implemented the changes. The id is stored in this._key with getters and setters on this.key. However, we can replace that with this.props.key, but I wanted to avoid unexpected regressions here.

This adds the functionality that if children.length === 1, no array is being passed to React, thus this doesn't need keys in that case. I think this is a nice parallel to how React works.

Furthermore this doesn't add uuids to keep it simple and to avoid a new dependency.

FunkMonkey · 2016-09-13T18:30:55Z

lib/Element.js

  var props = assign({}, this.props)
  props.style = assign({}, props.style)
-  props.key = isUndefined(props.key) ? idPrefix + '-' + index : props.key
+
+  if (isUndefined(props.key) && id) {


@philpl I think we're also missing a check for this._key and need to set props.key to this._key if necessary. Otherwise this._key has never been used. Right?

[edit] well. we still use key and thus this._key later, but this._key will not be used if we call toReact directly and set a key before...

var el = ReactFauxDOM.createElement('div'); el.key = 'foobar'; el.toReact();

I'm really not sure about _ prefixed methods + getters/setters. I think
there's a much simpler way. I'll reply with a PR or something soon.

On 13 September 2016 at 19:30, FunkMonkey notifications@github.com wrote:

In lib/Element.js
#73 (comment):

var props = assign({}, this.props)
props.style = assign({}, props.style)

props.key = isUndefined(props.key) ? idPrefix + '-' + index : props.key

if (isUndefined(props.key) && id) {

@philpl https://github.com/philpl I think we're also missing a check
for this._key and need to set it to props.key if necessary. Otherwise
this._key has never been used. Right?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Olical/react-faux-dom/pull/73/files/8a5f8837612a0ee97ba73a56073f2fe837c4a925..120f8f10bff6468fec5f92c41e3b7c765c2cd407#r78617435,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATPXav-Kt_k3RuT4IXJwkgCoxssSJk3ks5qpuvggaJpZM4J33UN
.

The key is now generated all the time in the constructor instead of on demand, since we apparently shouldn't add underscored, "private" properties, unless we really need them.

kitten · 2016-09-14T00:35:51Z

@Olical I've removed the getter and setter for this.key being set in the constructor. Thus this gets rid of the this._key property, but doesn't have the "on demand" element.

@FunkMonkey I think the code before confused you, if I'm not mistaken? props.key was being set with the id in toReact(id), if it's undefined. If the children are a list the parent passes the child's el.key to el.toReact. el.key would be a getter that generates the key on demand, and stores it in _key. The latest commit removes the on-demand part.

There a lots of solutions here. But what we ultimately implement is more a matter of taste, I believe. The parent could of course just set el.props.key directly, but I was thinking that should be the child's concern. This would make potential refactors of the API more pleasant too.

Also it nicely allows the user to pass a key to toReact as well, since setting props.key is probably not always something, that the user would like to do? It's obviously easy to change, so just hit me up with your thoughts.

FunkMonkey · 2016-09-14T06:58:17Z

@philpl I indeed confused myself. I remembered @Olical's comment wrong, where he proposed being able to also set a key by el.setAttribute('key', 'foo'). I remembered it as setting it using el.key = foo, which would have needed special treatment. Thus my wrong comment.

Anyway. Otherwise it looks good to me.

There is only slight caveat: If a library like d3 uses el.key or el.setAttribute('key', 'some data') by 'accident' for other reasons as they don't have React in mind. Doing any of both could seriously confuse React. On the other hand, an attribute called key will be stripped from the DOM anyway (just like custom attributes and there is no custom attribute support in sight), thus the application that requires a real DOM attribute called key (for CSS or whatever reason) will probably fail anyway. So I guess there is nothing to be done here...

kitten · 2016-09-14T09:13:07Z

@FunkMonkey I think there's no library that sets a key attribute on DOM Elements, if I'm not mistaken? If that ever becomes a problem, we could open another issue and replace the attribute with this.key appropriately.

FunkMonkey · 2016-09-14T18:10:41Z

@philpl No, not a library, but maybe users could:

d3.select('body').attr('key', 'F1');

But yeah. It's a quite negligible problem that isn't easy to solve either. Thus no need to fix.

kitten · 2016-09-14T21:09:46Z

@FunkMonkey I think it's not even an issue. If the user explicitly specifies a key, why not let him? ;) (Even when it's technically an attribute and not a prop)

FunkMonkey · 2016-09-18T17:19:33Z

@philpl Well I agree. It's not a real issue as it won't happen often and there is not really anything we can do anyway.

Just for clarification though, the problem with this code

d3.select( fauxDiv ).attr('key', 'F1');
d3.select( fauxDiv ).attr('some-attr', 'value');

is that due to React the result (if the fauxDiv has not been changed otherwise) is

<div>

and not

<div key="F1" some-attr="value">

because React will strip all custom attributes and also key. Even if we let the user ;)

Anyway. @Olical: You have been quiet lately. Is the current solution something you consider merging?

Olical · 2016-09-19T17:59:57Z

Sorry, I've been slacking on the programming front outside of work recently. I do feel like the children.length === 1 optimization may be premature however. I am still of the opinion that we should not be doing this, that we should let React warn the user (as it always has) and they can add meaningful keys, as you are supposed to. Otherwise wouldn't React do this anyway?

Please correct me if I'm wrong, I feel like I'm missing something. I regretted adding the automatic keys in the first place, so to see the concept being developed further causes me to pause and try convince myself that it is the right direction.

FunkMonkey · 2016-09-19T19:30:28Z

@Olical

I just tested it:

// case A
var inner = <div>Foo</div>;
return <div>{inner}</div>;

// case B
var inner = <div>Foo</div>;
return <div>{ [inner] }</div>;

Case B will trigger React's key warning, even though it is only a single child. Case A obviously doesn't.

Please correct me if I'm wrong, I feel like I'm missing something. I regretted adding the automatic keys in the first place, so to see the concept being developed further causes me to pause and try convince myself that it is the right direction.

I do strongly believe that automatic key generation is necessary for everything that is not the root level faux element. Consider the following case taken form here:

var distances = [2.23, 2.39, 2.59, 2.77];
const fauxRoot = ReactFauxDOM.createElement( 'div' );

d3.select( fauxRoot )
  .selectAll( 'p' )
    .data( distances )
    .enter( )
      .append( 'p' )
      .text( d => d + ' miles' );

const fauxRootReact = fauxRoot.toReact( );

return <div>{fauxRootReact}</div>

Without automatic key generation this example would inevitably output the React key warning, because the fauxRootReact divelement has 4 p children. None of them has a key prop assigned by d3 and I don't think we should expect the user to - otherwise using existing d3 code and libraries will be a pain. Also see that fauxRootReact itself doesn't have a key prop as it doesn't need one in the given situation. It will also not use its unique key. The current implementation makes this possible.

Did this clear it up for you?

Olical · 2016-09-20T08:36:43Z

So for the first example, this would happen with plain React too. It's a list of children, even if it has one item, React doesn't care. This is good and how it should be, if React had a special case for one child there would be more "special" cases lurking around every corner. I don't particularly like special cases because I feel they impair simplicity.

I know I'm probably frustrating you (sorry!) by not going with the PR just yet, but I'm trying to be sure of the right course since going back on things is far harder than initiating them 😄 please bear with me. Here's a corgi to tide you over.

I'm going to make a branch where I remove the automatic keys, then write tests to illustrate my side. If that fails entirely I'll have more of a understanding of your approach though. I want the best solution for everyone while maintaining simplicity and quality of the software.

kitten · 2016-09-20T09:06:27Z

@Olical Case A doesn't generate an array of children ;) That's an optimisation that is explicitly in React, to prevent an unnecessary array allocation.

You can see it in action in the createElement method: https://github.com/facebook/react/blob/c8fbdac22717e91d1f1638a932eed3ce4cc39ff7/src/isomorphic/classic/element/ReactElement.js#L196

Olical

I really want to like this change. I see your issues, but the global stateful UUID just scares me a little. It prevents me from efficiently generating my DOM on every render by being state that's out of my control.

Olical · 2016-09-23T16:43:12Z

lib/Element.js

+    })
+  }
+
+  return React.createElement(this.nodeName, props, children)


This is where I would like to introduce my little refactor, simplifying and moving into it's own function: https://github.com/Olical/react-faux-dom/pull/74/files#diff-52cea43ae897a1705ec51162aed25f63R276

Can do. Are you sure you want to add the function after its occurrence?

Well, it's hoisted regardless. I would move it to another file and require it but it becomes a cyclic dependency :(

You could add it above if you want, I don't mind.

Olical · 2016-09-23T16:45:48Z

lib/utils/uniqueID.js

+// for our use case
+function uniqueID () {
+  return 'faux-dom-' + (index++)
+}


What I don't like is that this requires you to build your element and keep it outside of your render method. It implies that you can not generate your element on the fly on every render (which I how I intended the library to be used). I mean, you can do that but you'll have new ids every time so it'll be slow.

I just don't like the statefulness of it, personally. I build everything stateless unless state is required because it leads to cleaner and less buggy code that's easier to read in six months time.

I mean, you can do that but you'll have new ids every time so it'll be slow.

Are we sure about that? Is it going to be slower? Anyway. Maybe there is a more deterministic method of creating the keys instead of using a global counter. Though I don't think there is a stateless way, considering my next point ...

I just don't like the statefulness of it, personally.

I do understand this. In your use-case the statefulness is not really necessary. In other user-cases, where faux DOM nodes are moved around, there is not way around it...

I build everything stateless unless state is required because it leads to cleaner and less buggy code

State is required in this case. I'm totally for minimising state, but in this case you're just handing the task of managing that state to your users.

The point is, this is not perfect when we're creating elements on the fly, but that's when the users should help out and provide keys themselves -- which then doesn't make a difference to what you're proposing. React's heuristic 1-1 diffing of lists without keys is also really sub optimal.

But when we're creating elements beforehand, the user doesn't have to do anything. This is a huge step forward.

I'd focus on how we can either try to remove our keys when we're creating elements on the fly, or how we can output nice warnings in that case, or both.

Olical · 2016-09-24T12:01:19Z

test/react.js

-  t.equal(tree.props.children[0].key, 'faux-dom-0')
+  t.plan(6)
+  t.equal(tree.key, 'test')
+  t.notEqual(tree.props.children[0].key, undefined)


This test is now pretty vague, all of the notEqual ones are. It could be equal to the entire works of Shakespeare and still pass 😢

Olical · 2016-09-24T12:01:32Z

lib/Element.js

+    })
+  }
+
+  return React.createElement(this.nodeName, props, children)


Well, it's hoisted regardless. I would move it to another file and require it but it becomes a cyclic dependency :(

You could add it above if you want, I don't mind.

Olical · 2016-09-24T12:03:41Z

lib/Element.js

@@ -245,20 +248,15 @@ Element.prototype.getBoundingClientRect = function () {
  return this.component.getBoundingClientRect()
 }

-Element.prototype.toReact = function (index) {
-  index = index || 0
+Element.prototype.toReact = function (id) {


So what's stopping us using my refactored "single node" aware recursion and leaving the IDs as they are? All functionality remains the same, single elements don't require keys and the user can set a different key if they need to place two children side by side. I fail to see why the global UUID is required to fix the original issue, could we not keep the stateless key approach and just document how to change the top level keys?

Phil Plückthun added 2 commits September 8, 2016 11:53

Add automatic unique keys for connectFauxDOM mixin

1096958

Since multiple core mixins can have identical keys, this generates an increasing ID.

kitten added 2 commits September 12, 2016 21:09

Remove automatic IDs from core mixin

8a5f883

FunkMonkey reviewed Sep 13, 2016
View reviewed changes

Remove key getter and setter for property in constructor

5531e62

The key is now generated all the time in the constructor instead of on demand, since we apparently shouldn't add underscored, "private" properties, unless we really need them.

kitten changed the title ~~Generate appended IDs recursively~~ Fix Element keys being just an index and children always being arrays Sep 14, 2016

kitten changed the title ~~Fix Element keys being just an index and children always being arrays~~ Fix automatic keys and Elements with a single child Sep 14, 2016

Olical mentioned this pull request Sep 20, 2016

Remove automatic keys #74

Closed

Olical reviewed Sep 23, 2016

View reviewed changes

Olical reviewed Sep 24, 2016

View reviewed changes

Olical mentioned this pull request Nov 6, 2016

'toReact' should not set default React 'key' #67

Closed

Olical closed this Dec 29, 2016

Olical reopened this Dec 29, 2016

Olical closed this Dec 29, 2016

Fix automatic keys and Elements with a single child #73

Fix automatic keys and Elements with a single child #73

Conversation

kitten commented Sep 8, 2016

Olical commented Sep 8, 2016 • edited Loading

kitten commented Sep 8, 2016 • edited Loading

FunkMonkey commented Sep 11, 2016

kitten commented Sep 11, 2016

FunkMonkey commented Sep 11, 2016

kitten commented Sep 12, 2016 • edited Loading

Olical commented Sep 12, 2016

FunkMonkey commented Sep 12, 2016 • edited Loading

Olical commented Sep 12, 2016 • edited Loading

kitten commented Sep 12, 2016 • edited Loading

FunkMonkey commented Sep 12, 2016 • edited Loading

kitten commented Sep 12, 2016

Olical commented Sep 12, 2016

kitten commented Sep 12, 2016 • edited Loading

FunkMonkey commented Sep 12, 2016

kitten commented Sep 12, 2016 • edited Loading

FunkMonkey Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitten commented Sep 14, 2016

FunkMonkey commented Sep 14, 2016 • edited Loading

kitten commented Sep 14, 2016

FunkMonkey commented Sep 14, 2016

kitten commented Sep 14, 2016

FunkMonkey commented Sep 18, 2016 • edited Loading

Olical commented Sep 19, 2016

FunkMonkey commented Sep 19, 2016 • edited Loading

Olical commented Sep 20, 2016

kitten commented Sep 20, 2016

Olical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olical Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olical commented Sep 8, 2016 •

edited

Loading

kitten commented Sep 8, 2016 •

edited

Loading

kitten commented Sep 12, 2016 •

edited

Loading

FunkMonkey commented Sep 12, 2016 •

edited

Loading

Olical commented Sep 12, 2016 •

edited

Loading

kitten commented Sep 12, 2016 •

edited

Loading

FunkMonkey commented Sep 12, 2016 •

edited

Loading

kitten commented Sep 12, 2016 •

edited

Loading

kitten commented Sep 12, 2016 •

edited

Loading

FunkMonkey Sep 13, 2016 •

edited

Loading

FunkMonkey commented Sep 14, 2016 •

edited

Loading

FunkMonkey commented Sep 18, 2016 •

edited

Loading

FunkMonkey commented Sep 19, 2016 •

edited

Loading

Olical Sep 23, 2016 •

edited

Loading