Refactor enzyme to use Adapters, initial React 16 support #1007

lelandrichardson · 2017-06-26T00:04:48Z

to: @ljharb @aweary @nfcampos @blainekasten
cc: @sebmarkbage @developit @bvaughn

This PR is effectively a rewrite of Enzyme's internal machinery, per the discussion outlined in #742

I attempted to make this as small of a breaking change as pragmatically possible, and minimize the amount our current unit tests needed to be changed across the refactor.

Motivation

This PR attempts to address a handful of pain points that Enzyme has been
subject to for quite a while.

The desired results are the following:

Cleaner code, easier maintenance, less bug prone.

By standardizing on a single tree specification, the implementation of Enzyme would no longer have
to take into account the matrix of supported structures and nuanced differences between different
versions of React, as well as to some extent the differences between mount and shallow.

Additional libraries can provide compatible adapters

React API-compatible libraries such as preact and inferno will be able to provide adapters to Enzyme
for their corresponding libraries, and be able to take full advantage of Enzyme's APIs.

Better user experience (ie, bundlers won't complain about missing deps)

Enzyme has had a long-standing issue with static-analysis bundlers such as Webpack and Browserify because
of our usage of internal React APIs. With this change, this is minimized to the adapter implementations only, which will be imported separately, so bundlers will not have any of these problems.

Standardization and interopability with other tools

With an agreed on tree format, other tools can start to use and
understand this format as well. Standardization is a good thing, and could allow tools to be built that maybe
don't even exist yet.

Things left to do before merging this PR:

clarify changes in documentation
complete migration guide
add in support for keys

Things left to do before publishing Enzyme 3.0:

Followup PR moving the adapters into their own packages
Publish alpha(s), get enzyme users to try it out and report back any issues (Airbnb at least will have to do this)
Implement new configurable top level API (debatable whether or not this is needed to publish 3.0)

ljharb · 2017-06-26T05:53:31Z

docs/future/compatibility.md

+to take into account the matrix of supported structures and nuanced differences between different
+versions of React, as well as to some extent the differences between `mount` and `shallow`.
+
+2. Additional libraries can provide compatible adapters


(nbd but you can use 1. for all of these, and then reordering won't require a diff on the entire list)

true. i usually avoid this because when reading it in plain text it gets confusing

ljharb · 2017-06-26T05:54:16Z

docs/future/migration.md

+```
+
+```js
+expect(wrapper.props()).to.deep.equal({ outer: x });


should this example show the same thing with shallow, to underscore the parallel?

yes. this document isn't done yet

ljharb · 2017-06-26T05:55:17Z

docs/future/migration.md

+
+## Refs
+
+Refs no longer return a "wrapper". They return what the ref would actually be.


this should elaborate on what the options are (iirc, null/undefined or a DOM node?)

or a composite component instance?

ljharb · 2017-06-26T05:55:26Z

docs/future/migration.md

+
+## Keys
+
+keys no longer work? we should maybe fix this in the spec...


add a TODO for this?

there already is one in the PR description

ljharb · 2017-06-26T05:56:40Z

docs/future/migration.md

+
+## for shallow, getNode() was renamed to getElement()
+
+## for mount, getNode() should not be used. instance() does what it used to.


I'm confused - getNode() should have been returning a DOM node, and instance() should return an instance of a component. Has this changed for mount?

If we're renaming getNode on shallow, let's do the same for mount - such that neither has getNode or .node or .nodes anymore (so people's tests will fail and they can do any needed migration)

ljharb · 2017-06-26T06:38:12Z

src/adapters/ReactSixteenAdapter.js

+import React from 'react';
+import ReactDOM from 'react-dom';
+import ReactDOMServer from 'react-dom/server';
+// import TestRenderer from 'react-test-renderer';


let's make sure to do a sweep and remove all unneeded commented-out code before merging :-)

ljharb · 2017-06-26T06:41:46Z

src/adapters/EnzymeAdapter.js

+  );
+}
+
+class EnzymeAdapter {


i don't see anywhere we're checking instanceof EnzymeAdapter - it seems like we'd want to enforce people use this interface.

Whats the strategy for exposing this class for adapter authors? Will it just be accessible from a lib folder like

import EnzymeAdapter from 'enzyme/lib/EnzymeAdapter'

If we use it and enforce it, it'd need to be its own package so things could depend on it, and semver would help identify incompatibilities sooner.

ljharb · 2017-06-26T06:42:51Z

src/ShallowWrapper.js

@@ -1078,13 +1100,14 @@ class ShallowWrapper {
  dive(options = {}) {
    const name = 'dive';
    return this.single(name, (n) => {
-      if (isDOMComponentElement(n)) {
+      if (n && n.nodeType === 'host') {
        throw new TypeError(`ShallowWrapper::${name}() can not be called on DOM components`);


should this phrase - "DOM components" - come from the adapter, and not from enzyme itself?

I'm just going to change the terminology to "Host Components". This PR moves enzyme to an adapter architecture, but removing concepts of react entirely from enzyme and making it possible to use enzyme for react-like libraries is close, but will require some additional refactoring i'm sure. For things like terminology of "DOM Components" etc, I think these ideas are fairly baked in in other parts of the code as well. I think that that's okay at this point.

ljharb · 2017-06-26T06:43:16Z

src/ShallowWrapper.js

@@ -684,6 +699,11 @@ class ShallowWrapper {
        'a context option',
      );
    }
+    if (this.instance() === null) {
+      throw new Error(
+        'ShallowWrapper::context() can only be called on class components as of React 16',


this seems like an error that should come from the react 16 adapter

ljharb · 2017-06-26T06:45:04Z

src/ReactWrapper.jsx

-    this.component.setChildProps(props, callback);
+    this.component.setChildProps(props, () => {
+      this.update();
+      callback();


we should probably check that it's a function here, just like on line 309

since we should check that it's a function (rather than letting an error be thrown async), should this still default to null or undefined?

no it should default to a noop because the previous behavior was that it wasn't required.

right - it's not required here tho either. defaulting it to undefined means we'd throw if a non-undefined non-function was provided.

i'm adding a typeof check before the setChildProps call. But keeping it defaulting to noop. Seem reasonable?

Sure, I guess - it just adds function call overhead in the case when nothing is provided, so i'm not sure why this is better than defaulting to undefined.

nfcampos · 2017-06-26T12:44:13Z

docs/future/compatibility.md

+## Motivation
+
+This proposal is attempting to address a handful of pain points that Enzyme has been
+subject to for quite a while.  This proposal has resulted mostly [#715](https://github.com/airbnb/enzyme/issues/715),


the second sentence seems to be missing some word somewhere, maybe "from" after "mostly"

nfcampos

first pass, I haven't looked through all files yet

nfcampos · 2017-06-26T12:45:23Z

docs/future/compatibility.md

+
+```js
+// Strings and Numbers are rendered as literals.
+type LiteralValue = string | number


If numbers are coerced to strings early on, will a LiteralValue ever be a number, or only a string?

this is react 16 specific behavior that i don't think we should put in the spec...

nfcampos · 2017-06-26T12:45:55Z

docs/future/compatibility.md

+// A "node" in an RST is either a LiteralValue, or an RSTNode
+type Node = LiteralValue | RSTNode
+
+// if node.type


what does this mean?

nfcampos · 2017-06-26T12:48:16Z

docs/future/compatibility.md

+
+  // For a given node, this corresponds roughly to the result of the `render` function with the 
+  // provided props, but transformed into an RST. For "host" nodes, this will always be `null` or
+  // an Array. For "composite" nodes, this will always be `null` or an `RSTNode`.


with react 16 can't this also be an array of RSTNode?

nfcampos · 2017-06-26T12:51:21Z

docs/future/compatibility.md

+  // An optional predicate function that takes in an `Element` and returns
+  // whether or not the underlying Renderer should treat it as a "Host" node
+  // or not. This function should only be called with elements that are 
+  // not required to be considered "host" nodes (ie, with a string `type`),


this bit sounds confusing, it makes me consider that this function should be renamed to something like shouldConsiderCompositeAsHost?

nfcampos · 2017-06-26T12:52:46Z

docs/future/compatibility.md

+}
+
+type EnzymeAdapter = {
+  // This is a method that will return a semver version string for the _react_ version that


_react_ -> `react`

nfcampos · 2017-06-26T13:08:38Z

docs/future/migration.md

+
+## Refs
+
+Refs no longer return a "wrapper". They return what the ref would actually be.


or a composite component instance?

nfcampos · 2017-06-26T13:17:30Z

docs/future/migration.md

+wrapper.find('.async-btn').simulate('click');
+setImmediate(() => {
+  // this didn't used to be needed
+  wrapper.update(); // TODO(lmr): this is a breaking change...


this is a pretty big breaking change that we should try very hard to avoid

Hoping this doesn't fall through the cracks -- is there something about the new architecture that prevents an adapter from always grabbing from the latest output, or node, whenever a traversal starts? (i.e. myRootWrapper.find(...))

yes, the new architecture makes this fundamentally not possible. I don't see any way around this.

I was able to get this working as a POC: jwbay@df609cf

Since the wrapper .getNode functions are already the starting access point for all traversals, we can just get the latest root node there.

The only downside is we lose reference equality for nodes during traversal in some cases now that there's always a new object literal created in the adapter's getNode. I tried caching based on render output but it didn't make much of a difference because most traversals end up going through .wrap, which creates a new renderer.

It's probably also worth noting that mount now also requires .update in places it didn't before. This new test fails in this branch but passes in master: jwbay@c14e87b

nfcampos · 2017-06-26T13:17:49Z

docs/future/migration.md

+
+
+
+## Enzyme.use


what's this? Enzyme.configure?

nfcampos · 2017-06-26T13:19:23Z

docs/future/migration.md

+- x make sure all react dependence is moved into adapters
+- x make tests run for all adapters
+- export tests for 3rd party adapters to run
+- check the targetApiVersion returned by the adapter and use the semver library


use it for what? maintain a map of react api versions to available api methods? and warn/throw accordingly if an unavailable method is used?

nfcampos · 2017-06-26T13:21:28Z

package.json

@@ -21,13 +21,14 @@
    "test:watch": "mocha --recursive --watch test",
    "test:karma": "karma start",
    "test:env": "sh ./example-test.sh",
-    "test:all": "npm run react:13 && npm run test:only && npm run react:14 && npm run test:only && npm run react:15.4 && npm run test:only && npm run react:15 && npm run test:only",
+    "test:all": "npm run react:13 && npm run test:only && npm run react:14 && npm run test:only && npm run react:15.4 && npm run test:only && npm run react:15 && npm run test:only && npm run react:16 && npm run test:only",
    "clean-local-npm": "rimraf node_modules/.bin/npm node_modules/.bin/npm.cmd",
    "react:clean": "npm run clean-local-npm && rimraf node_modules/react node_modules/react-dom node_modules/react-addons-test-utils node_modules/react-test-renderer && npm prune",


add create-react-class

doesn't create-react-class not need to be removed because it'd jut be ignored in older versions?

I hope that’s true yes, but I’d rather test on environments closer to what an actual person eg. using react 13 would have (ie. no create react class installed)

milesj · 2017-06-26T18:40:50Z

setupAdapters.js

+let Adapter = null;
+
+if (Version.REACT013) {
+  Adapter = require('./src/adapters/ReactThirteenAdapter');


What if enzyme went with the Lerna approach and published individual packages for each version? So if I only wanted react15, I would install the enzyme and enzyme-adapter-react15 packages?

isn't that the plan?

Perhaps? Doesn't look to be from this PR.

It's not part of this PR, but it's absolutely the plan. See "Followup PR moving the adapters into their own packages" in the OP.

Ah thanks, missed that sentence.

aweary · 2017-06-27T00:37:18Z

docs/future/compatibility.md

+  lineNumber: number 
+|}
+
+type NodeType = 'class' | 'function' | 'host';


The current toTree implementation in react-test-renderer only uses "component" and "host" for nodeType.

aweary · 2017-06-27T00:43:25Z

docs/future/compatibility.md

+import Enzyme from 'enzyme';
+import ThirdPartyEnzymeAdapter from 'third-party-enzyme-adapter';
+
+Enzyme.configure({ adapter: ThirdPartyEnzymeAdapter });


.configure would make more sense if there were long-term plans to provide other top-level configuration options via Enzyme.configure. Otherwise 👍 for .use

aweary · 2017-06-27T01:23:59Z

docs/future/compatibility.md

+
+### Validation
+
+Enzyme will provide an `validate(node): Error?` method that will traverse down a provided `RSTNode` and 


What node would Enzyme call it with at Enzyme.configure?

aweary · 2017-07-03T19:05:11Z

docs/future/compatibility.md

+  // etc. For react adapters, this will likely just be `() => React.Version`, but for other
+  // adapters for libraries like inferno or preact, it will allow those libraries to specify
+  // a version of the API that they are committing to.
+  getTargetApiVersion(): string;


If we just required that adapters be explicitly configured, would we need this getTargetApiVersion API at all? What happens if a React-like adapter (Preact, Inferno) exists for a version that collides with a React version? e.g., Preact has a 15.5 release.

yeah. i think you're right. I think I will get rid of this, and let node semver declarations take care of the rest.

aweary · 2017-07-03T19:25:48Z

src/ShallowWrapper.js

-  // React <15.5: Fallback to ReactDOM
-  return batchedUpdates(fn);
+  const adapter = configuration.get().adapter;
+  // TODO(lmr): warn about no adapter being configured


Should this throw instead of warn?

aweary · 2017-07-03T19:27:52Z

src/ShallowWrapper.js

-          ) {
-            instance.componentDidMount();
-          }
+      this.renderer = getAdapter(options).createRenderer({ mode: 'shallow', ...options });


If mode is being used by Enzyme internals should it be added to the RendererOptions type definition?

aweary · 2017-07-03T19:32:32Z

src/Utils.js

-    }
-  }
-
+function propsOfNode(node) {


RSTTraversal exports a propsOfNode utility, should we unify these?

perhaps, yes. I've been trying to separate things mentally between adapters and the core enzyme lib so that they are easy to separate, and it's easier for me to find when things are no longer used

aweary · 2017-07-17T17:41:28Z

What do you all think about adding pointers for nextSibling and prevSibling? I found that having sibling pointers made it much easier when doing later traversal. For example, checking adjacent or general siblings. Since we're already traversing the whole tree, it should just require a constant amount of extra time to attach these pointers.

ljharb · 2017-07-17T20:48:00Z

@aweary doesn't that open up the possibility that nextSibling or prevSibling could be wrong? In other words, it gives the adapter the ability to hijack the traversal algorithm (or am I misunderstanding)

aweary · 2017-07-17T21:25:31Z

@ljharb we already trust that the adapter is returning an accurate RSTNode, so that doesn't sound like a problem to me. If we update the RSTNode type to include sibling pointers we're just extending the contract we're already requiring of adapters.

TiraO · 2017-07-24T20:40:52Z

@lelandrichardson I tried this out on my tests and I ran into a couple of issues:

I had to call Enzyme.configure. I don't think users should be forced to call Enzyme.configure by default. Enzyme should work out-of-the-box. Also, require('enzyme/setupAdapters') in my test helper file didn't apply the configuration for some reason.
Explicitly calling update looks like it isn't updating child props correctly (haven't really dug into this)
simulate (on a non-root node) is throwing an error Error: ReactWrapper::update() can only be called on the root

The following test passes against master:

    it('should simulate events on child nodes', ()=>{
      class Foo extends React.Component {
        constructor(props) {
          super(props);
          this.state = { count: 0 };
          this.incrementCount = this.incrementCount.bind(this);
        }

        incrementCount() {
          this.setState({ count: this.state.count + 1 });
        }

        render() {
          return (
            <div>
              <a
                className={`clicks-${this.state.count}`}
                onClick={this.incrementCount}
              >
                foo
              </a>
            </div>
          );
        }
      }

      const wrapper = mount(<Foo />);

      expect(wrapper.find('.clicks-0').length).to.equal(1);
      wrapper.find('a').simulate('click');
      expect(wrapper.find('.clicks-1').length).to.equal(1);
    });

ljharb · 2017-07-24T23:53:08Z

@TiraO "working out of the box by default" is the current state, and it's why we have issues with dependencies. It's important that users explicitly choose the version of React (or "not React") they're using.

TiraO · 2017-07-25T17:04:19Z

@ljharb Fair enough. Would there be both a react 16 native adapter and a react 16 dom adapter?

ljharb · 2017-07-26T06:08:56Z

@TiraO it would be possible to create one for each; i'm not sure what we're planning on releasing with, but the whole concept is that it's extensible.

koba04 · 2017-08-04T09:20:19Z

Is [email protected] based on this PR?

What about merging this PR into a branch like v3 and releasing alpha or beta versions from the branch?
It's helpful to track the progress of supporting React v16 and I can work on supporting React v16.

ljharb · 2017-08-15T04:26:52Z

docs/future/migration.md

+with "React-like" libraries such as Preact and Inferno.
+
+We have done our best to make Enzyme v3 as API compatible with v2.x as possible, however there are
+a hand full of breaking changes that we decided we needed to make, intentionally, in order to


s/hand full/handful

ljharb · 2017-08-15T04:27:45Z

docs/future/migration.md

+on React 16, you would want to configure Enzyme this way:
+
+```js
+import Enzyme from 'enzyme';


should this example use import { configure } from 'enzyme', even though both ways will work?

aesthetically i kinda prefer this way for some reason, but i don't think i can justify it in any way :P

ljharb · 2017-08-15T04:28:42Z

docs/future/migration.md

+```
+
+In this code, there is a timer that continuously changes the rendered output of this component. This
+might be a reasonable thing to do in your application. The thing is, Enzyme has no way of knowing


do we want to lowercase enzyme throughout?

ljharb · 2017-08-15T04:36:17Z

src/RSTTraversal.js

@@ -110,7 +105,7 @@ export function buildPredicate(selector) {
          return node => nodeHasId(node, selector.slice(1));

        case SELECTOR.PROP_TYPE: {
-          const propKey = selector.split(/\[([a-zA-Z-]*?)(=|])/)[1];
+          const propKey = selector.split(/\[([a-zA-Z][a-zA-Z_\d\-:]*?)(=|])/)[1];


Those tests are passing just fine on master :-/ if you revert this change, does everything still pass?

ljharb · 2017-08-15T04:42:54Z

src/ReactWrapper.jsx

-
-      eventFn(findDOMNode(n), mock);
+      this.renderer.simulateEvent(n, event, mock);
+      this.root.update();


is this guaranteed to happen after the simulated event is finished?

nothing really is guaranteed other than simulatedEvent() finishing here... which will call the event I believe (at least our tests indicate), but of course the event could schedule something async

ljharb · 2017-08-15T04:43:32Z

src/ReactWrapper.jsx

@@ -600,7 +657,7 @@ class ReactWrapper {
   * @returns {ReactWrapper}
   */
  children(selector) {
-    const allChildren = this.flatMap(n => childrenOfInst(n.getNode()));
+    const allChildren = this.flatMap(n => childrenOfNode(n.getNodeInternal()).filter(x => typeof x === 'object'));


it'd be nice to have a named isObject function, or, to have childrenOfNode take an arg, or have a variant, that only returns non-primitives

ljharb

Looks good to merge - we've discussed some post-PR pre-v3 additional tasks, including removing some internal properties; restructuring the adapters into a multi-package-repo structure; adding .root() and making mount's and shallow's methods consistent about what the wrapper "is" conceptually; and addressing any extant TODOs and PR comments.

Thanks for all this work, v3 is going to be great!

zachlendon · 2017-08-16T00:48:28Z

Any sense when a release might be made available with this change?

ljharb · 2017-08-16T00:53:07Z

None yet.

midastouchprd · 2017-08-30T20:56:46Z

any update on release with these changes?

lelandrichardson added the semver: major Breaking changes label Jun 26, 2017

lelandrichardson force-pushed the lmr--adapter-impl branch 3 times, most recently from 5e069dc to c98e97c Compare June 26, 2017 01:34

ljharb reviewed Jun 26, 2017

View reviewed changes

nfcampos reviewed Jun 26, 2017

View reviewed changes

jpshelley mentioned this pull request Jun 26, 2017

Support React 16 apollographql/react-apollo#760

Closed

ljharb mentioned this pull request Jun 26, 2017

Add enzyme/webpack entrypoint with getPluginsForInstalledReact function #895

Closed

milesj reviewed Jun 26, 2017

View reviewed changes

aweary reviewed Jul 3, 2017

View reviewed changes

aweary mentioned this pull request Jul 6, 2017

Remove functions marked as undocumented and unused facebook/react#10096

Closed

sebmarkbage mentioned this pull request Jul 7, 2017

[Fiber] Umbrella for remaining features / bugs facebook/react#7925

Closed

aweary mentioned this pull request Jul 7, 2017

Remove unused isCompositeComponentElement re-export #1023

Closed

gaearon mentioned this pull request Jul 7, 2017

React 16 Umbrella ☂️ (and 15.5) facebook/react#8854

Closed

lelandrichardson force-pushed the lmr--adapter-impl branch from c98e97c to 538550f Compare July 16, 2017 20:08

ljharb mentioned this pull request Jul 17, 2017

Support React 16 #928

Closed

ljharb mentioned this pull request Jul 20, 2017

add support to specify max level to shallow/mount #1042

Closed

ljharb mentioned this pull request Jul 25, 2017

Remove warnings when rendering react-native components #831

Closed

aweary mentioned this pull request Jul 27, 2017

React 16 RC facebook/react#10294

Closed

ljharb mentioned this pull request Jul 31, 2017

Incorrect dependency version for React needed for React Native 0.43 #893

Closed

koba04 mentioned this pull request Aug 4, 2017

Shallow renderer never call componentDidUpdate in v16. #1058

Closed

lelandrichardson added 8 commits August 14, 2017 20:24

remove need for getElement

bd93716

more PR feedback

6966eda

PR feedback

c87313b

remove need for childrenToArray

2a7c32a

add adapter methods

12a79c1

refactor getNode and getNodes

568ce89

finishing touches

3cc005d

lint warnings

04c4a67

lelandrichardson force-pushed the lmr--adapter-impl branch from a315d72 to 04c4a67 Compare August 15, 2017 03:37

lelandrichardson added 2 commits August 14, 2017 21:58

fix tests

f85156d

fix lint

95950fa

ljharb reviewed Aug 15, 2017

View reviewed changes

strange react 16 error boundary issues

e1d4551

ljharb approved these changes Aug 15, 2017

View reviewed changes

ljharb added API: mount API: render API: shallow labels Aug 15, 2017

lelandrichardson merged commit 165a521 into master Aug 15, 2017

lelandrichardson deleted the lmr--adapter-impl branch August 15, 2017 07:02

This was referenced Aug 15, 2017

[RFC] Enzyme Adapter + React Standard Tree Proposal #742

Closed

TypeError: (0 , _reactCompat.createShallowRenderer) is not a function #1070

Closed

ljharb mentioned this pull request Aug 17, 2017

find(MyStatelessFunctionalComponent) fails when running on minified react-dom #1041

Closed

bkazi mentioned this pull request Aug 30, 2017

Enzyme compatibility preactjs/preact-compat#82

Closed

ljharb mentioned this pull request Sep 1, 2017

wrapper.instance() is always undefined #1104

Closed

mathisonian mentioned this pull request Sep 8, 2017

React 16 idyll-lang/idyll#144

Closed

paranoidjk mentioned this pull request Sep 11, 2017

Add Preact test ant-design/ant-design-mobile#1831

Closed

jimmy-devine mentioned this pull request Sep 11, 2017

v3 Timeline #1109

Closed


		## Refs

		Refs no longer return a "wrapper". They return what the ref would actually be.


		## Keys

		keys no longer work? we should maybe fix this in the spec...


		## for shallow, getNode() was renamed to getElement()

		## for mount, getNode() should not be used. instance() does what it used to.


		### Validation

		Enzyme will provide an `validate(node): Error?` method that will traverse down a provided `RSTNode` and

Refactor enzyme to use Adapters, initial React 16 support #1007

Refactor enzyme to use Adapters, initial React 16 support #1007

Conversation

lelandrichardson commented Jun 26, 2017 • edited by ljharb Loading

Motivation

Cleaner code, easier maintenance, less bug prone.

Additional libraries can provide compatible adapters

Better user experience (ie, bundlers won't complain about missing deps)

Standardization and interopability with other tools

Things left to do before merging this PR:

Things left to do before publishing Enzyme 3.0:

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

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

nfcampos 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

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

milesj Jun 26, 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

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

aweary commented Jul 17, 2017

ljharb commented Jul 17, 2017

aweary commented Jul 17, 2017

TiraO commented Jul 24, 2017 • edited Loading

ljharb commented Jul 24, 2017

TiraO commented Jul 25, 2017

ljharb commented Jul 26, 2017

koba04 commented Aug 4, 2017

Choose a reason for hiding this comment

lelandrichardson commented Jun 26, 2017 •

edited by ljharb

Loading

milesj Jun 26, 2017 •

edited

Loading

TiraO commented Jul 24, 2017 •

edited

Loading