Add warning for rendering into container that was updated manually #10210

flarnie · 2017-07-18T20:54:37Z

Edit: This is ready for review.

what is the change?:
We want to warn when users render into a container which was manually
emptied or updated outside of React. This can lead to the cryptic error
about not being able to remove a node, or just lead to silent failures
of render. This warning should make things more clear.

Note that this covers the case where the contents of the root container
are manually updated, but does not cover the case where something was
manually updated deeper in the tree.

why make this change?:
To maintain parity and increase clarity before releasing v16.0 beta.

test plan:
yarn test

issue:
#8854

last item under the '16 beta' checklist.

flarnie · 2017-07-18T23:32:11Z

Problem: When we render using ReactDOM.unstable_createPortal it renders into the portal dom node, and so the HostInstance parentNode is not the container. I'm looking for a way to detect that and skip the warning in that case. ReactPortal.isPortal looks useful but I'm not finding the thing to call it on.

sophiebits

lgtm. are we also planning to warn/throw in the removeChild method?

sophiebits · 2017-07-18T23:30:36Z

src/renderers/dom/fiber/ReactDOMFiberEntry.js

+          'render(...): It looks like the content of this container may have ' +
+          'been updated or cleared outside of React. ' +
+          'This can cause errors or failed updates to the container. ' +
+          'Please call `ReactDOM.render` with your new content.',


How about:

render(...): It looks like the React-rendered content of this container was removed without using React. This is not supported and will cause errors. Instead, call ReactDOM.unmountComponentAtNode to empty a container.

since it's not likely that it is simple for people to switch to using React to render things – we just want them to use React to clean things up properly.

sophiebits · 2017-07-18T23:31:06Z

src/renderers/dom/fiber/ReactDOMFiberEntry.js

+      const hostInstance =
+        DOMRenderer.findHostInstance(container._reactRootContainer.current);
+      if (hostInstance) {
+        warned = true;


this variable doesn't do anything? I think it's fine to deduplicate but also fine not to

sophiebits · 2017-07-18T23:31:49Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

+
+    it('should not warn when rendering into an empty container', () => {
+      spyOn(console, 'error');
+      ReactDOM.render(<div>'foo'</div>, container);


(did you mean to render quotes into the string? it doesn't much matter but maybe you just wanted <div>foo</div> here)

sophiebits · 2017-07-18T23:32:20Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

+    });
+
+    it('should warn when replacing a container which was manually updated outside of React', () => {
+       spyOn(console, 'error');


(indentation)

sophiebits · 2017-07-18T23:32:56Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

+      expect(() => {
+        container.innerHTML = '<div>MEOW.</div>';
+        ReactDOM.render(<div key='2'>'baz'</div>, container);
+      }).toThrow();


can you assert on the error text here to make sure the right error is thrown? I think passing a string to toThrow will work.

Good idea - will fix.

sophiebits · 2017-07-18T23:35:05Z

unstable_createPortal doesn't use this code path at all, does it? it's a new feature that won't be used frequently so I'm not too concerned about it as long as it's not broken

flarnie · 2017-07-18T23:41:35Z

"unstable_createPortal doesn't use this code path at all, does it?"

You are right, that function itself does not use this code path. It's just that patterns which arise when using that method can lead to this warning firing. Specifically, https://github.com/facebook/react/blob/master/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js#L760-L798

it's a new feature that won't be used frequently so I'm not too concerned about it as long as it's not broken

Awesome :) - for now I'll stub out 'console.error' in that test.

sophiebits · 2017-07-18T23:44:46Z

Wait, I misunderstood. We should fix this. (Things warning incorrectly counts as "broken" in my book.)

sophiebits · 2017-07-18T23:45:21Z

The issue is that findHostInstance returns the node inside the portal if that's the first child?

flarnie · 2017-07-19T00:16:11Z

The issue is that findHostInstance returns the node inside the portal if that's the first child?

That's what it looks like. Will verify in a bit.

flarnie · 2017-07-19T00:59:48Z

Yea, the exact problem is that hostInstance.parentNode is the portalContainer passed to ReactDOM.unstable_createPortal and not the container passed to ReactDOM.render originally.

I was hoping that children was a portal in this case, but it is more like children is a component that returns a portal from render.

flarnie · 2017-07-19T01:18:53Z

So we set the _reactRootContainer attribute on the container as we render, and would it be ok to set a _reactPortalContainer attribute on the portal in unstable_createPortal? It would not need any info in particular, I could either store the same thing that we put in _reactRootContainer or just an empty object that might be handy for the future.

flarnie · 2017-07-19T01:30:58Z

Going to work more on this in the morning and hopefully get those last couple of tests passing. Tips and feedback are very welcome - this is one of the last three blockers for 16.0 beta~ 🎊

flarnie · 2017-07-19T16:45:12Z

are we also planning to warn/throw in the removeChild method?

I think that would be nice, but the 'removeChild' method is a "hot path" and based on what @sebmarkbage was saying we might want to either omit that warning or find a different place for it.

I don't think we need that warning in place for a 16 beta release, but we can discuss if you feel strongly about it.

gaearon · 2017-07-19T17:41:53Z

src/renderers/shared/fiber/isomorphic/ReactPortal.js

@@ -27,6 +27,7 @@ exports.createPortal = function(
  implementation: any,
  key: ?string = null,
 ): ReactPortal {
+  containerInfo._reactPortalContainer = {};


This seems unexpected to me. createPortal looks like a pure function (like createElement) and I wouldn’t expect it to put a property on its argument. Can we do this when the portal mounts for the first time instead? I’m not sure where this code would go though. This is probably not a big deal in grand scheme of things but it does feel odd.

Another concern is it might already exist. So this always allocates an object. I think we should create at most one object.

It would make me feel better about it if this was a boolean rather than an object, and if we only set it in __DEV__. Then it’s clear it shouldn’t be relied on, and it‘s only used for warning rather than keeping data or important logic. In this case I don’t mind side effect that much.

I also wonder if this means createPortal(<div />, null) is going to throw (because containerInfo is null but is being accessed). Which I think ideally it should, but I’m not sure if this is the case now. If we currently silently skip it, it might break product code that accidentally relies on this. I would expect master to still throw in this case though, just later. But we need to verify. Ideally we’d add an early invariant in the future but that seems out of scope.

Good points;

Absolutely we should check for the presence of '_reactPortalContainer' before adding another one. Will fix.

I can look for a place add the '_reactPortalContainer' that is more similar to where the '_reactRootContainer' is added.

Will wrap the '_reactPortalContainer' addition in a __DEV__ check.

I could rename '_reactPortalContainer' to 'unstable_reactPortalContainer' to additionally show that it's not to be relied on.

Agreed in general that it feels odd; if there is another way to determine whether the parent is a portal I'd rather use existing code, but didn't see anything else that was exposed there.

I also wonder if this means createPortal(<div />, null) is going to throw (because containerInfo is null but is being accessed). Which I think ideally it should, but I’m not sure if this is the case now.

That sounds about right to me, and I also would expect an error if I tried to pass 'null' as the container to createPortal. Hopefully this is a corner case, and if product code relied on it they could add a null check.

We only have one internal use case and in that framework they currently create a new div element for the portalContainer and then they do set it to null when unmounting the component.

So if somehow createPortal was called after the portal component unmounted that would be a problem. I don't think that is possible with the current code but maybe it's worth adding a warning?

Absolutely we should check for the presence of '_reactPortalContainer' before adding another one. Will fix.

It seems like if we change this to be a boolean then it’s not a problem and we can always set it. Does it have to be an object?

I could rename '_reactPortalContainer' to 'unstable_reactPortalContainer' to additionally show that it's not to be relied on.

I think we use unstable_ for actual APIs people might call. In this case something like __reactInternalIsPortalContainer would look better IMO.

So if somehow createPortal was called after the portal component unmounted that would be a problem.

I don’t think this is possible. So seems fine.

cc @sebmarkbage unifying containers and portals might provide a less hacky solution to this.

This is in renderer agnostic code but the warning is only in the DOM. This would mutate any portal that's not a DOM node which is not great. EDIT: This can be fixed by moving it out here:

react/src/renderers/dom/fiber/ReactDOMFiberEntry.js

Line 718 in 357925a

// TODO: pass ReactDOM portal implementation as third argument

It seems like if we change this to be a boolean then it’s not a problem and we can always set it. Does it have to be an object?

Right - making it a boolean now, and so that no longer needs fixed. I guess I meant that in the case that it was an object we should do that.

I had thought that an object would be useful if we ever want to store more metadata on the portal container - but there is no reason to make it an object until then.

I think we use unstable_ for actual APIs people might call. In this case something like __reactInternalIsPortalContainer would look better IMO.

That makes sense, thanks for the context! Will fix the name.

I'm still looking for where to assign the __reactInternalIsPortalContainer property, and will push another commit shortly that fixes that and the other issues. Thanks for the dialog about this.

sebmarkbage · 2017-07-19T19:54:16Z

src/renderers/dom/fiber/ReactDOMFiberEntry.js

+    if (container._reactRootContainer && container.nodeType !== COMMENT_NODE) {
+      const hostInstance = DOMRenderer.findHostInstance(
+        container._reactRootContainer.current,
+      );


As suggested by @spicyj we could expose another method here that doesn't drill through portals. The problem with that is dead code elimination. It's a bit awkward that we expose a new method on the public Reconciler API that is only ever needed for a specific DEV warning.

If it would be possible to write such a method, without adding a flag to the portalContainer node, then there must already be something we could look at here that would tell us if it's a portal. Is that right?

No because the portal is between the container and the hostInstance.

HostRoot -> ... -> HostPortal -> ... -> HostComponent

You'd have to traverse the Fibers to figure that out.

Ah, ok. Interesting.

What is the advantage of adding another method to DOMRenderer rather than setting a boolean flag on the portalContainer node?

This lets us not mutate the node in what seems to be a pure function.

Does it help that we now do the mutating inside of ReactFiberCommitWork? That is where we do other DOM updates.

If we really want to avoid adding this flag then I can implement another method on DOMRenderer but I think @sebmarkbage's point that "It's a bit awkward that we expose a new method on the public Reconciler API that is only ever needed for a specific DEV warning." is valid, and I'd rather avoid that.

In a situation like this: render([<Portal />,<div />], container) we'd find the portal node and then ignore checking the second one. So no warning if we mutated container. If we did @spicyj's suggestion, we could find all the direct children or at least the first one.

Good point - talked more with @spicyj offline also and it looks like we may also just be able to change this spot to filter portals out, and then we might not need a new method?

react/src/renderers/shared/fiber/ReactFiberTreeReflection.js

Line 245 in 7b9f643

// TODO: If we hit a Portal, we're supposed to skip it.

sebmarkbage · 2017-07-20T01:02:43Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -218,6 +218,10 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
          }
        } else {
          if (isContainer) {
+            if (parentFiber.tag === HostPortal) {
+              // allows us to identify the portal container in other places
+              parent.__reactInternalIsPortalContainer = true;


This is in shared renderer code but this expando is DOM specific. We shouldn't assume that all renderers are able to handle expandos. It should also be DEV only since it has perf considerations.

flarnie · 2017-07-20T17:43:48Z

It seems much better to filter out portals in findHostInstance, and that seems to work.

CI might fail due to an "obsolete snapshot", I am trying to find and remove it now.

RFC because we still need to tidy this up and verify that all tests pass. **what is the change?:** We want to warn when users render into a container which was manually emptied or updated outside of React. This can lead to the cryptic error about not being able to remove a node, or just lead to silent failures of render. This warning should make things more clear. Note that this covers the case where the contents of the root container are manually updated, but does not cover the case where something was manually updated deeper in the tree. **why make this change?:** To maintain parity and increase clarity before releasing v16.0 beta. **test plan:** `yarn test` **issue:** facebook#8854 last item under the '16 beta' checklist.

STILL TODO: figure out how to skip this warning when the component renders to a portal. Unfortunately 'ReactPortal.isPortal(children)' returns false, even in the failing test where we are rendering to a portal. **what is the change?:** - added a test for the case where we call 'ReactDOM.render' with a new container, using a key or a different type, after the contents of the first container were messed with outside of React. This case throws, and now at least there will be an informative warning along with the error. - We updated the check to compare the parent of the 'hostInstance' to the container; this seems less fragile - tweaked some comments **why make this change?:** Continue improving this to make it more final. **test plan:** `yarn test` **issue:** facebook#8854

**what is the change?:** See title **why make this change?:** See comment in the code **test plan:** `yarn test` **issue:** facebook#8854

**what is the change?:** We have a warning for cases when the container doesn't match the parent which we remembered the previously rendered content being rendered into. We are skipping that warning when you render into a 'comment' node. **why make this change?:** Basically, if you render into a 'comment' node, then the parent of the comment node is the container for your rendered content. We could check for similarity there but rendering into a comment node seems like a corner case and I'd rather skip the warning without knowing more about what could happen in that case. **test plan:** `yarn test` **issue:** facebook#8854

@spicyj

**what is the change?:** Various changes to get this closer to being finished; - Improved warning message (thanks @spicyj!!!) - Removed dedup check on warning - Remove mocking of 'console.error' in portals test **why make this change?:** - warning message improvement: communicates better with users - Remove dedup check: it wasn't important in this case - Remove mocking of 'console.error'; we don't want to ignore an inaccurate warning, even for an "unstable" feature. **test plan:** `yarn test` -> follow-up commits will fix the remaining tests **issue:** issue facebook#8854

**what is the change?:** Add a property to a container which was rendered into using `ReactDOM.unstable_createPortal`. **why make this change?:** We don't want to warn for mismatching container nodes in this case - the user intentionally rendered into the portal container instead of the original container. concerns; - will this affect React Native badly? - will this add bloat to the portal code? seems small enough but not sure. **test plan:** `yarn test` **issue:** facebook#8854

**what is the change?:** When focusing on fixing the warning to not check when we are using portals, I missed checking for the existence of the host instance parent before checking if it was a portal. This adds the missing null checks. **why make this change?:** To fix a bug that the previous commit introduced. **test plan:** `yarn test` -> follow-up commits fix more of the test failures **issue:** facebook#8854

**what is the change?:** - removed extra single quotes, downgrade double quotes to single - update expected warning message to match latest warning message - fix indentation **why make this change?:** - get tests passing - code maintainability/readability **test plan:** `yarn test` follow up commits will fix the remaining tests **issue:** facebook#8854

…d markup **what is the change?:** We have a test that verifies React can reconcile text from pre-rendered mark-up. It tests React doing this for three strings and three empty strings. This adds a call to 'unmountComponentAtNode' between the two expectations for strings and empty strings. **why make this change?:** We now warn when someone messes with the DOM inside of a node in such a way that removes the React-rendered content. This test was doing that. I can't think of a situation where this would happen with server-side rendering without the need to call 'unmountComponentAtNode' before inserting the server-side rendered content. **test plan:** `yarn test` Only one more failing test, will fix that in the next commit. **issue:** facebook#8854

**NOTE:** I am still looking for a good place to move this flag assignment to, or a better approach. This does some intermediate fixes. **what is the change?:** - fixed flow error by allowing optional flag on a DOMContainer that indicates it was used as a portal container. - renamed the flag to something which makes more sense **why make this change?:** - get Flow passing - make this change make more sense We are still not sure about adding this flag; a follow-up diff may move it or take a different approach. **test plan:** `yarn test` **issue:** facebook#8854

**what is the change?:** We add a flag to the container of a 'portal' in the 'commit work' phase in Fiber. This is right before we call `appendChildToContainer`. **why make this change?:** - Sometimes people call `ReactDOM.render(... container)`, then manually clear the content of the `container`, and then try to call another `ReactDOM.render(... container)`. - This leads to cryptic errors or silent failure because we hold a reference to the node that was rendered the first time, and expect it to still be inside the container. - We added a warning for this issue in `renderSubtreeIntoContainer`, but when a component renders something returned by `ReactDOM.unstable_createPortal(<Component />, portalContainer);`, then the child is inside the `portalContainer` and not the `container, but that is valid and we want to skip warning in that case. Inside `renderSubtreeIntoContainer` we don't have the info to determine if a child was rendered into a `portalContainer` or a `container`, and adding this flag lets us figure that out and skip the warning. We originally added the flag in the call to `ReactDOM.unstable_createPortal` but that seemed like a method that should be "pure" and free of side-effects. This commit moves the flag-adding to happen when we mount the portal component. **test plan:** `yarn test` **issue:** facebook#8854

**what is the change?:** This is awful. :( I'm not sure how else to let Flow know that we expect that this might be a sort of `DOMContainer` type and not just a normal `Node` type. To at least make the type information clear we added a comment. **why make this change?:** To get `flow` passing. Looks like we have `any` types sprinkled throughout this file. phooey. :( **test plan:** `yarn flow` **issue:** facebook#8854

**what is the change?:** We want to ignore portals when firing a certain warning. This allows us to get the host instance and ignore portals. Also added a new snapshot recording while fixing things. **why make this change?:** Originally we had added a flag to the DOM node which was used for rendering the portal, and then could notice and ignore children rendered into those nodes. However, it's better to just ignore portals in `DOMRenderer.findHostInstance` because - we will not ignore a non-portal second child with this approach - we meant to ignore portals in this method anyway (according to a 'TODO' comment) - this change only affects the DOM renderer, instead of changing code which is shared with RN and other renderers - we avoid adding unneeded expandos **test plan:** `yarn test` **issue:** facebook#8854

flarnie · 2017-07-20T18:01:58Z

I think there is a bug where an empty snapshot is treated as an 'outdated snapshot'.

If I delete the obsolute snapshot, and run ReactDOMFiber-test.js it generates a new snapshot:

but then when I run the test with the newly generated snapshot, it says "1 obsolete snapshot found":

After this gets landed, at some point, will file an issue with Jest. For now going to skip the snapshot generation for the error message in the new test.

I think there is a bug where an empty snapshot is treated as an 'outdated snapshot'. If I delete the obsolute snapshot, and run ReactDOMFiber-test.js it generates a new snapshot. But then when I run the test with the newly generated snapshot, it says "1 obsolete snapshot found", At some point I will file an issue with Jest. For now going to skip the snapshot generation for the error message in the new test.

gaearon · 2017-07-20T19:24:20Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -218,6 +218,10 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
          }
        } else {
          if (isContainer) {
+            if (parentFiber.tag === HostPortal) {
+              // allows us to identify the portal container in other places
+              parent.__reactInternalIsPortalContainer = true;


This can be removed now?

ack - thought I had gotten that. Thanks. Removing now.

**what is the change?:** see title **why make this change?:** this is part of an old approach to detecting portals, and we have instead added a check in the `findHostInstance` method to filter out portals. **test plan:** `yarn test` **issue:** facebook#8854

needs more review

sophiebits

lgtm

flarnie · 2017-07-20T20:41:34Z

lol thank you

sebmarkbage

I don't think we should change the findDOMNode API to ignore Portal because then it's not safe to add/remove a Portal in the middle of the tree.

sophiebits · 2017-07-20T20:53:42Z

People don't hesitate before or removing a wrapping div. Why is this so different?

flarnie · 2017-07-20T21:08:58Z

Just chatted with @sebmarkbage and we realized there is an issue with filtering out portals in findHostInstance:

the findHostInstance method is one of the methods that will be available to anyone writing a renderer in the future, and we can't really assume that they will want to filter out portals. When I think of common use cases, it seems likely that they will in fact want to be able to find the host instance for a portal.
this also affects the public findDOMNode method, and again it seems likely that some folks will want to use this method with portals in the future.

We decided that it's still better to have a method to call rather than adding an expando, and so I'm going to update this to fork DOMRenderer.findHostInstance to be something like DOMRenderer.findHostInstanceWithNoPortals.

sebmarkbage · 2017-07-20T21:15:13Z

Other than this change, a Portal is virtual. It's one less thing that can go wrong.

With fragments, hopefully wrapping divs are less common. When they are used they have useful semantics that triggers the change. It has a benefit. For the findDOMNode in Portal case, it doesn't seem to me that having this be different gives you any benefit.

It doesn't seem like this is actually how you'd want findDOMNode to work. Seems like the motivation here was to share code, not that it was the best semantics.

sophiebits · 2017-07-20T21:17:42Z

Basically, I don't like findDOMNode and want to push people away from it by making it less useful.

**what is the change?:** We need to get host instances, but filter out portals. There is not currently a method for that. **why make this change?:** Rather than change the existing `findHostInstance` method, which would affect the behavior of the public `findDOMNode` method, we are forking. **test plan:** `yarn test` **issue:** facebook#8854

flarnie · 2017-07-21T15:48:41Z

@sebmarkbage if you have any follow-up requests I will be happy to do them in a follow-up PR.

flarnie requested review from sophiebits and sebmarkbage July 18, 2017 20:54

facebook-github-bot added the CLA Signed label Jul 18, 2017

sophiebits previously approved these changes Jul 18, 2017

View reviewed changes

flarnie force-pushed the testForEmptyingContainerOutsideOfReact branch from 5e36329 to 02b1d01 Compare July 19, 2017 00:53

flarnie changed the title ~~RFC Add warning for rendering into container that was updated manually~~ Add warning for rendering into container that was updated manually Jul 19, 2017

flarnie requested a review from sophiebits July 19, 2017 16:43

gaearon reviewed Jul 19, 2017

View reviewed changes

sebmarkbage reviewed Jul 19, 2017

View reviewed changes

flarnie force-pushed the testForEmptyingContainerOutsideOfReact branch from 3a09245 to 9dd038c Compare July 19, 2017 22:42

gaearon mentioned this pull request Jul 19, 2017

React 16 Umbrella ☂️ (and 15.5) #8854

Closed

sebmarkbage reviewed Jul 20, 2017

View reviewed changes

flarnie added 7 commits July 20, 2017 10:45

Stub our console.error in one of the portal tests

a439ba6

**what is the change?:** See title **why make this change?:** See comment in the code **test plan:** `yarn test` **issue:** facebook#8854

flarnie added 10 commits July 20, 2017 10:45

ran prettier

a2fdfd2

remove unused variable

48f8f83

run scripts/fiber/record-tests

64ce334

Ran prettier

e64c2c1

flarnie force-pushed the testForEmptyingContainerOutsideOfReact branch from 3c1b07e to e64c2c1 Compare July 20, 2017 17:52

gaearon reviewed Jul 20, 2017

View reviewed changes

sophiebits approved these changes Jul 20, 2017

View reviewed changes

sebmarkbage requested changes Jul 20, 2017

View reviewed changes

sophiebits approved these changes Jul 20, 2017

View reviewed changes

flarnie merged commit 1f74eca into facebook:master Jul 21, 2017

aweary mentioned this pull request Jul 28, 2017

React 16 RC #10294

Closed

Add warning for rendering into container that was updated manually #10210

Add warning for rendering into container that was updated manually #10210

Conversation

flarnie commented Jul 18, 2017 • edited Loading

flarnie commented Jul 18, 2017

sophiebits 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

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

sophiebits commented Jul 18, 2017

flarnie commented Jul 18, 2017

sophiebits commented Jul 18, 2017

sophiebits commented Jul 18, 2017

flarnie commented Jul 19, 2017

flarnie commented Jul 19, 2017

flarnie commented Jul 19, 2017

flarnie commented Jul 19, 2017

flarnie commented Jul 19, 2017

Choose a reason for hiding this comment

gaearon Jul 19, 2017 • 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

sebmarkbage Jul 19, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flarnie commented Jul 20, 2017

flarnie commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits left a comment

Choose a reason for hiding this comment

flarnie commented Jul 20, 2017

sebmarkbage left a comment

Choose a reason for hiding this comment

sophiebits commented Jul 20, 2017

flarnie commented Jul 20, 2017

sebmarkbage commented Jul 20, 2017

sophiebits commented Jul 20, 2017

flarnie commented Jul 21, 2017

flarnie commented Jul 18, 2017 •

edited

Loading

gaearon Jul 19, 2017 •

edited

Loading

sebmarkbage Jul 19, 2017 •

edited

Loading

sebmarkbage Jul 20, 2017 •

edited

Loading