Add stack unwinding phase for handling errors #12201

acdlite · 2018-02-09T23:51:51Z

A rewrite of error handling, with semantics that more closely match stack unwinding.

Errors that are thrown during the render phase unwind to the nearest error boundary, like before. But rather than synchronously unmount the children before retrying, we restart the failed subtree within the same render phase. The failed children are still unmounted (as if all their keys changed) but without an extra commit.

Commit phase errors are different. They work by scheduling an error on the update queue of the error boundary. When we enter the render phase, the error is popped off the queue. The rest of the algorithm is the same.

This approach is designed to work for throwing non-errors, too, though that feature is not implemented yet.

Known differences from existing error handling semantics in synchronous mode:

When a component throws an error, we continue rendering its siblings rather than unwind immediately.
Because of the previous point, a single error boundary may capture more than one error during a single render, if multiple child branches fail.
We call componentDidCatch for each error that a boundary captures, instead of just the first one.
Error boundaries only catch error objects. Non-errors will propagate to the root and cause the tree to unmount, as if it’s an uncaught error.

Unresolved items:

Add getDerivedStateFromCatch lifecycle behind a feature flag. This is fired during the render phase so that we can retry without committing, as opposed to componentDidCatch, which fires in the commit phase. We'll keep componentDidCatch around for logging purposes. If getDerivedStateFromCatch is not implemented but componentDidCatch is, the fallback behavior is to unmount all the boundary's children. (This part is already implemented.) We may warn about the missing getDerivedStateFromCatch in a future release.
Existing code that schedules recovery updates inside componentDidCatch will have mostly the same semantics, with one key exception. We no longer track whether an error boundary failed in a previous commit. So it's now possible to fall into a recursive error loop, in the same way as an unguarded setState inside componentDidUpdate.

acdlite · 2018-02-09T23:53:17Z

packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js

@@ -784,7 +788,7 @@ describe('ReactErrorBoundaries', () => {
    ]);
  });

-  it('propagates errors on retry on mounting', () => {
+  xit('propagates errors on retry on mounting', () => {


This is related to item 2 in the "Unresolved items" section. Waiting for feedback on whether this is too drastic a change and if we should hew more closely to the existing retry semantics.

bvaughn · 2018-02-09T23:57:06Z

Before reviewing, I'm curious: are these changes intended for (or necessary for) 16.3?

I ask because the change is kind of large, and I'm wondering what the impact on timing would be (e.g. time to review, sync and test internally, update more documentation, etc.)

gaearon · 2018-02-09T23:57:56Z

Existing code that schedules recovery updates inside componentDidCatch will have mostly the same semantics, with one key exception. We no longer track whether an error boundary failed in a previous commit. So it's now possible to fall into a recursive error loop, in the same way as an unguarded setState inside componentDidUpdate.

If we were to do this, should the boundary also handle its own errors? Our justification for not doing it was that this leads to loops. But now we have loops anyway?

acdlite · 2018-02-09T23:59:49Z

@gaearon Talked it through with @sebmarkbage. What we'll do is keep the failedErrorBoundaries tracking around for legacy purposes, until we can migrate people to the new strategy in React 17. Unless the boundary defines getDerivedStateFromCatch, which opts you into the new behavior.

gaearon · 2018-02-10T00:02:15Z

packages/react-reconciler/src/ReactFiberIncompleteWork.js

+  if (value instanceof Error) {
+    algebraicEffectTag = Err;
+  } else if (value !== null && typeof value === 'object') {
+    // If instanceof fails, fall back to duck typing.


This feels surprising. Moving code into an iframe could fundamentally change the behavior if it used to pass the first check but doesn't pass the second one now.

Maybe we should only do duck typing and skip instanceof entirely?

This would make sense to me. I would maybe check just stack since it's added by the browser, but message could potentially be empty (I think?)

On the other hand is it plausible that Promise ever gets a stack?

Also, what happens if you just throw a string? It's bad but it can happen. We used to handle this, do we not anymore?

My impression is that the plan is to treat thenables specially, and treat everything else as an error. Why is the logic written around special casing the error object?

I want to leave the door open for supporting custom algebraic effects. Error boundaries are a normal effect boundary that pattern matches on errors; there’s another boundary that pattern matches on promises, and so on. That’s what these lines here are intended to model.

Error boundaries will no longer catch null or undefined, but the root still will (that’s why the Unknown type exists).

Do error boundaries still catch strings?

No they only catch errors. A string will propagate to the root. I’ll add this to the list of known deviations.

If this is too drastic a change I’m fine with for holding off on that part until React 17. Though if we do that, we should warn so people don’t rely on that behavior.

To be honest I'd say it's okay to say this was unsupported. It's a known bad pattern, and error boundaries should not (yet :-) be used for control flow anyway. We always suggested they're meant to be used for exceptional situations.

I don't think this is quite ok for a breaking change. The cross-realm issue and the fact that throwing strings, while not good practice, is still common.

Thenables are a lot more restrictive since they will essentially always unwrap through any async boundary, they're pretty useless for anything other than as promises. So limiting this to thenable would be a lot safer.

I understand that our general strategy is to attempt to limit componentDidCatch to errors only but not quite sure about the mechanism here.

acdlite · 2018-02-10T00:03:47Z

@bvaughn It would nice to get this into 16.3 because the changes we're planning for 16.4 depend on it.

gaearon · 2018-02-10T00:05:58Z

packages/react-reconciler/src/ReactFiberRoot.js

@@ -42,6 +39,9 @@ export type FiberRoot = {
  pendingContext: Object | null,
  // Determines if we should attempt to hydrate on the initial mount
  +hydrate: boolean,
+  // Remaining expiration time on this root.
+  // TODO: List this into the renderer


gaearon · 2018-02-10T00:08:23Z

packages/react-reconciler/src/ReactFiberScheduler.js

+        let thrownValue = value;
+        // Check if the thrown value is an error. If it is, we will replay the
+        // begin phase using invokeGuardedCallback, so that the error is not
+        // treated as caught by the debugger.


gaearon · 2018-02-10T00:09:27Z

packages/react-reconciler/src/ReactFiberScheduler.js

+            typeof value.stack === 'string')
+        ) {
+          // Retore the original state of the work-in-progress
+          Object.assign(workInProgress, stashedWorkInProgressProperties);


Not a super big fan of this. I usually grep for assignments when debugging to figure out what piece of code changed a field. On the other hand the code is smaller..

I don't love it either but all the alternatives I considered were worse. I can try to convince you out of band if needed :D

#12502 (comment)

gaearon · 2018-02-10T00:11:24Z

packages/react-reconciler/src/ReactFiberScheduler.js

+    // We assume any use of `deferredUpdates` is to split a high-priority
+    // interactive update into separate high and low values. Set this to true so
+    // that the low-pri value is treated as if it were the result of an
+    // interaction, even if we're in the render phase.


I don't understand this, can you explain more? Does this fix the issue I reported or is it unrelated?

Meant to delete this, not needed

gaearon · 2018-02-10T00:14:12Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js

+      'BadMount',
+      'componentDidCatch: Error 1',
+      'componentDidCatch: Error 2',
+      'componentDidCatch: Error 3',


This makes sense theoretically but how practical is it? Once we're in an error state it's likely other things can break due to mistaken assumptions with cryptic errors that can't be reproduced otherwise.

I guess a logger could remember whether it saw an error earlier, and attach that as metadata.

I think in most cases, if you catch multiple errors, it will be because they can from different siblings. (Another difference between this implementation and the previous one is that if there's an error, we continue rendering the siblings before we unwind.) In that case, you'd want to log all three errors.

~~Realizing this test made more sense when it was using the render phase version of the lifecycle. I'll rewrite.~~ Never mind, I still think this makes sense. Three siblings, three errors.

I think it makes sense to start dealing with this because it is less confusing than sometimes getting different errors depending on which sibling got furthest.

gaearon · 2018-02-10T00:17:02Z

packages/shared/ReactTypeOfSideEffect.js

+
+// Bits greater than Incomplete are used to identify fibers that threw. These
+// are like algebraic effects. These bits are mutually exclusive.
+export const Combined = /*              */ 0b010000000000;


Can you explain why this is called "combined"? What is being combined here? Isn't the first effect that we "catch" the only one? Or do we attempt to render siblings as deep as we can and then "wait on all"?

I think I'll just get rid of this, but the idea was that you could dispatch multiple errors at the same time. It's only being used for unhandled errors at the root. Don't think it's necessary.

acdlite · 2018-02-10T02:58:01Z

If you're looking for a place to start, most of the interesting stuff happens in a new module called ReactFiberIncompleteWork. This is where we handle unwinding an incomplete component stack. https://github.com/acdlite/react/blob/1f38a7b1b5acb3c2b7a3a075c5358b934ef6d50e/packages/react-reconciler/src/ReactFiberIncompleteWork.js

gaearon · 2018-02-10T14:00:42Z

So.. this is kinda huge.

I understand it's a prerequisite for blockers so we probably have to do it. But is there any way we can reduce the amount of code here? Or is there a plan for how we can do it later?

How "easy" would blockers be after this, in terms of code size?

gaearon · 2018-02-10T14:26:25Z

It's less dramatic with error extraction (compared against master without extraction).

Still pretty large.

acdlite · 2018-02-10T18:31:09Z

The biggest code additions are the ReactFiberIncompleteWork module, which handles stack unwinding, and the uncommenting of resumeMountClassInstance, since error boundaries are now a form of resuming. Both overlap heavily with planned features, but the flip side of that is if we weren’t planning to eventually add those, this PR could be structured in a different way to make the code smaller.

There are several places where some more abstraction (or better abstraction) could cut down on repetition and reduce code size, particularly in ReactFiberClassComponent. But I’d like to receive feedback on the overall architecture before I start optimizing too much.

gaearon · 2018-02-10T22:06:23Z

packages/react-reconciler/src/ReactFiberIncompleteWork.js

+        break;
+      case IndeterminateComponent:
+        break;
+      default:


Is this code path commonly exercised in tests? Is there a risk that we'll add a new tag, forget to add it to this switch, and it blows up only on the error path (which is rare enough that we will miss it)?

It’s triggered whenever anything throws, so we need test cases that throw for every possible type. Working on adding this to the fuzz tester.

Seems like a good candidate for a Flow exhaustiveness check.

True. Though we still should test every path to prevent stack push/pop mismatch bugs, like we’ve seen in the past.

sebmarkbage · 2018-02-12T18:29:48Z

packages/react-reconciler/src/ReactFiber.js

@@ -139,6 +139,8 @@ export type Fiber = {|
  firstEffect: Fiber | null,
  lastEffect: Fiber | null,

+  thrownValue: any,


You'll have to make the case for this addition.

Strongest case I think is resuming. We should be able to reuse parts of the effect list during a bail out, including any of the values that were thrown.

Can you think of some viable alternatives? I'm having trouble coming up with one.

sebmarkbage · 2018-02-12T19:22:51Z

packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js

-  it('does not break with a bad Map polyfill', () => {
+  // We don't currently use fibers as keys. Re-enable this test if we
+  // ever do again.
+  it.skip('does not break with a bad Map polyfill', () => {


Does it matter having this enabled? We will definitely not enable this if we ever do again. :)

(Which might be this diff instead of the extra field.)

Yeah, I can reenable, but if we ever do this again, we'll have to ensure that this test triggers whatever path adds a fiber to Map/Set. So I think we'll need to think about it regardless.

sebmarkbage · 2018-02-12T19:24:47Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js

+      'BadMount',
+      'componentDidCatch: Error 1',
+      'componentDidCatch: Error 2',
+      'componentDidCatch: Error 3',


I think it makes sense to start dealing with this because it is less confusing than sometimes getting different errors depending on which sibling got furthest.

sebmarkbage · 2018-02-12T21:56:07Z

packages/react-dom/src/__tests__/ReactDOM-test.js

@@ -380,6 +380,7 @@ describe('ReactDOM', () => {
  it('throws in DEV if jsdom is destroyed by the time setState() is called', () => {
    class App extends React.Component {
      state = {x: 1};
+      componentDidUpdate() {}


What is this for?

There's a comment below. It's used to trigger invokeGuardedCallback, which no longer wraps the begin phase unless an error is thrown.

sebmarkbage · 2018-02-12T22:01:30Z

packages/react-reconciler/src/ReactFiberBeginWork.js

+    workInProgress.nextEffect = null;
+    workInProgress.firstEffect = null;
+    workInProgress.lastEffect = null;
+    workInProgress.thrownValue = null;


Why did this move? We should be careful not to add things to things that happen in the hottest paths unless they always need to happen. If they sometimes need to happen, it should only happen in those paths.

sebmarkbage · 2018-02-12T22:23:45Z

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -148,6 +163,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
        workInProgress,
        current.child,
        nextChildren,
+        deleteExistingChildren,


Like discussed offline. Let's call the reconciler twice instead of using boolean flag, first with nextChildren = null, then with current.child = null.

sebmarkbage · 2018-02-12T22:25:13Z

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -237,19 +253,22 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
    // During mounting we don't know the child context yet as the instance doesn't exist.
    // We will invalidate the child context in finishClassComponent() right after rendering.
    const hasContext = pushLegacyContextProvider(workInProgress);
+    const instance = workInProgress.stateNode;


You're not using the instance. Let's inline it back in place. It avoids reading the stateNode property here when current is not null.

sebmarkbage · 2018-02-12T22:40:45Z

packages/react-reconciler/src/ReactFiberScheduler.js

    fiber: Fiber,
+    startTime: ExpirationTime,


rm until we need it

sebmarkbage · 2018-02-13T01:36:30Z

packages/react-reconciler/src/ReactFiberIncompleteWork.js

+        break;
+      case Fragment:
+        break;
+      case Mode:


Should we just remove all these branches that do nothing?

I guess so. I thought maybe the “unknown tag” invariant would reduce the likelihood that we’d overlook one of the types but it doesn’t provide that much safety. Need tests regardless.

acdlite · 2018-02-20T12:54:02Z

@sebmarkbage Ready for another look

sebmarkbage · 2018-02-24T00:26:25Z

packages/react-reconciler/src/ReactFiberScheduler.js

+        // begin phase using invokeGuardedCallback, so that the error is not
+        // treated as caught by the debugger.
+        if (
+          value instanceof Error ||


Let's inverse this check to look for thenables instead.

I'll remove it entirely until the next PR

sebmarkbage · 2018-02-24T00:27:40Z

packages/react-reconciler/src/ReactFiberScheduler.js

+      expirationTime,
+    ) {
+      const stashedWorkInProgressProperties = Object.assign({}, workInProgress);
+      try {


Can we move this out of the hot loop and instead be around workLoop?

A rewrite of error handling, with semantics that more closely match stack unwinding. Errors that are thrown during the render phase unwind to the nearest error boundary, like before. But rather than synchronously unmount the children before retrying, we restart the failed subtree within the same render phase. The failed children are still unmounted (as if all their keys changed) but without an extra commit. Commit phase errors are different. They work by scheduling an error on the update queue of the error boundary. When we enter the render phase, the error is popped off the queue. The rest of the algorithm is the same. This approach is designed to work for throwing non-errors, too, though that feature is not implemented yet.

Fires during the render phase, so you can recover from an error within the same pass. This aligns error boundaries more closely with try-catch semantics. Let's keep this behind a feature flag until a future release. For now, the recommendation is to keep using componentDidCatch. Eventually, the advice will be to use getDerivedStateFromCatch for handling errors and componentDidCatch only for logging.

This way we don't have to store the thrown values on the effect list.

We don't need it yet. We'll reconsider once we add another exception type.

This moves it out of the hot path.

gaearon · 2018-03-21T14:13:04Z

packages/react-reconciler/src/ReactFiberCommitWork.js

+            const errorInfo = capturedErrors[i];
+            const error = errorInfo.value;
+            logError(finishedWork, errorInfo);
+            instance.componentDidCatch(error);


Did we intentionally "lose" the errorInfo argument here? Seems like a major change.

* Add stack unwinding phase for handling errors A rewrite of error handling, with semantics that more closely match stack unwinding. Errors that are thrown during the render phase unwind to the nearest error boundary, like before. But rather than synchronously unmount the children before retrying, we restart the failed subtree within the same render phase. The failed children are still unmounted (as if all their keys changed) but without an extra commit. Commit phase errors are different. They work by scheduling an error on the update queue of the error boundary. When we enter the render phase, the error is popped off the queue. The rest of the algorithm is the same. This approach is designed to work for throwing non-errors, too, though that feature is not implemented yet. * Add experimental getDerivedStateFromCatch lifecycle Fires during the render phase, so you can recover from an error within the same pass. This aligns error boundaries more closely with try-catch semantics. Let's keep this behind a feature flag until a future release. For now, the recommendation is to keep using componentDidCatch. Eventually, the advice will be to use getDerivedStateFromCatch for handling errors and componentDidCatch only for logging. * Reconcile twice to remount failed children, instead of using a boolean * Handle effect immediately after its thrown This way we don't have to store the thrown values on the effect list. * ReactFiberIncompleteWork -> ReactFiberUnwindWork * Remove startTime * Remove TypeOfException We don't need it yet. We'll reconsider once we add another exception type. * Move replay to outer catch block This moves it out of the hot path.

NE-SmallTown · 2018-04-22T07:34:43Z

When we enter the render phase, the error is popped off the queue. The rest of the algorithm is the same.

Should 'render phase' be 'commit phase'?

NE-SmallTown · 2018-04-22T15:39:20Z

packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js

@@ -1379,6 +1387,10 @@ describe('ReactErrorBoundaries', () => {
      // The initial render was aborted, so
      // Fiber retries from the root.
      'ErrorBoundary componentWillUpdate',
+      'ErrorBoundary componentDidUpdate',


@gaearon @acdlite Could you please tell me what does 'The initial render was aborted' mean? I'm a little confused that we call componentWillUpdate and componentDidUpdate but don't call the render method, this is different with the common life cycle process

* Add stack unwinding phase for handling errors A rewrite of error handling, with semantics that more closely match stack unwinding. Errors that are thrown during the render phase unwind to the nearest error boundary, like before. But rather than synchronously unmount the children before retrying, we restart the failed subtree within the same render phase. The failed children are still unmounted (as if all their keys changed) but without an extra commit. Commit phase errors are different. They work by scheduling an error on the update queue of the error boundary. When we enter the render phase, the error is popped off the queue. The rest of the algorithm is the same. This approach is designed to work for throwing non-errors, too, though that feature is not implemented yet. * Add experimental getDerivedStateFromCatch lifecycle Fires during the render phase, so you can recover from an error within the same pass. This aligns error boundaries more closely with try-catch semantics. Let's keep this behind a feature flag until a future release. For now, the recommendation is to keep using componentDidCatch. Eventually, the advice will be to use getDerivedStateFromCatch for handling errors and componentDidCatch only for logging. * Reconcile twice to remount failed children, instead of using a boolean * Handle effect immediately after its thrown This way we don't have to store the thrown values on the effect list. * ReactFiberIncompleteWork -> ReactFiberUnwindWork * Remove startTime * Remove TypeOfException We don't need it yet. We'll reconsider once we add another exception type. * Move replay to outer catch block This moves it out of the hot path.

acdlite requested review from bvaughn, sebmarkbage and gaearon February 9, 2018 23:51

facebook-github-bot added the CLA Signed label Feb 9, 2018

acdlite commented Feb 9, 2018

View reviewed changes

gaearon reviewed Feb 10, 2018

View reviewed changes

acdlite mentioned this pull request Feb 10, 2018

Capture and recover from errors within a single render phase #11901

Closed

5 tasks

gaearon reviewed Feb 10, 2018

View reviewed changes

acdlite force-pushed the error-handling-refactor branch from 2570d09 to dfe3ab3 Compare February 10, 2018 00:24

acdlite force-pushed the error-handling-refactor branch from 1f38a7b to 06b7db4 Compare February 10, 2018 03:16

gaearon reviewed Feb 10, 2018

View reviewed changes

sebmarkbage reviewed Feb 12, 2018

View reviewed changes

packages/react-reconciler/src/ReactFiberScheduler.js Outdated

fiber: Fiber,

startTime: ExpirationTime,

Copy link

Collaborator

sebmarkbage Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm until we need it

sebmarkbage reviewed Feb 15, 2018

View reviewed changes

acdlite force-pushed the error-handling-refactor branch 4 times, most recently from cee3941 to 401ab7b Compare February 20, 2018 12:02

acdlite force-pushed the error-handling-refactor branch 5 times, most recently from 2ca1f63 to d889f97 Compare February 20, 2018 12:52

sebmarkbage reviewed Feb 24, 2018

View reviewed changes

acdlite force-pushed the error-handling-refactor branch from ddac776 to 750bae0 Compare February 24, 2018 01:19

sebmarkbage approved these changes Feb 24, 2018

View reviewed changes

acdlite force-pushed the error-handling-refactor branch from 750bae0 to 521054b Compare February 24, 2018 01:31

acdlite added 8 commits February 23, 2018 17:35

Reconcile twice to remount failed children, instead of using a boolean

4540403

Handle effect immediately after its thrown

866e3a9

This way we don't have to store the thrown values on the effect list.

ReactFiberIncompleteWork -> ReactFiberUnwindWork

0f24e2c

Remove startTime

8a62bef

Remove TypeOfException

e021e0c

We don't need it yet. We'll reconsider once we add another exception type.

Move replay to outer catch block

559bbb5

This moves it out of the hot path.

acdlite force-pushed the error-handling-refactor branch from 521054b to 559bbb5 Compare February 24, 2018 01:37

acdlite merged commit 94518b0 into facebook:master Feb 24, 2018

gaearon reviewed Mar 21, 2018

View reviewed changes

gaearon mentioned this pull request Mar 21, 2018

Fix now-missing errorInfo argument to componentDidCatch() #12416

Merged

gaearon mentioned this pull request Mar 29, 2018

Disable createRoot for open source builds #12486

Merged

NE-SmallTown reviewed Apr 25, 2018

View reviewed changes

acdlite mentioned this pull request Nov 6, 2018

Recover from errors with a boundary in completion phase #14104

Merged

Add stack unwinding phase for handling errors #12201

Add stack unwinding phase for handling errors #12201

Conversation

acdlite commented Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

bvaughn commented Feb 9, 2018 • edited Loading

gaearon commented Feb 9, 2018

acdlite commented Feb 9, 2018

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

acdlite commented Feb 10, 2018

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

Choose a reason for hiding this comment

acdlite Feb 10, 2018 • 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

acdlite commented Feb 10, 2018

gaearon commented Feb 10, 2018

gaearon commented Feb 10, 2018 • edited Loading

acdlite commented Feb 10, 2018 • 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

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

Choose a reason for hiding this comment

acdlite commented Feb 20, 2018

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

NE-SmallTown commented Apr 22, 2018

NE-SmallTown Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

acdlite commented Feb 9, 2018 •

edited

Loading

bvaughn commented Feb 9, 2018 •

edited

Loading

acdlite Feb 10, 2018 •

edited

Loading

gaearon commented Feb 10, 2018 •

edited

Loading

acdlite commented Feb 10, 2018 •

edited

Loading

NE-SmallTown Apr 22, 2018 •

edited

Loading