Initial version of eps for ABI-Stable-Module-API #20

mhdawson · 2016-04-29T21:34:31Z

First cut of the ABI-Stable-Module-API eps. It is far
from complete but is intended to allow insight into the
current direction/status and will be updated regularly
as we progress along the investigation and can make
the API definition more complete.

mhdawson · 2016-04-29T21:45:22Z

FYI @nodejs/api, @nodejs/addon-api

kkoopa · 2016-04-29T22:17:10Z

00X-ABI-Stable-Module-API.md

+node::ni::SetPrototypeMethod(ni_env, recv, name, callback)
+node::ni::SetTemplate(ni_env,...)
+node::ni::SetPrototypeTemplate(ni_env,...)
+node::ni::SetInstanceTemplate(ni_env,...)


Most of these are fluff. Two functions should suffice: a Set(primitive_or_template) for templates and a Set(value) for objects.

Set(primitive_or_template) is probably still too V8-centric to work with e.g. the SM API for defining classes.

Not exposing templates would kill performance, would it not? I am not familiar with how SM does it, but from a pragmatic approach, should V8 not remain a first-class citizen?

To be clear, I'm not saying the V8 implementation shouldn't use templates, but following the V8 API too closely might make it awkward to implement it efficiently for other engines.

In terms of the boundary between the methods exported by the node.js binary and how the module calls them, we need a strong separation that I believe will limit the use of templating at that level. We can't have engine code being incorporated into the module and still achieve ABI stability.

In terms of the api itself, however, the use of templates should still be able to be used to make the API easier to use but the code pulled in from the templating would be limited to code that is part of the API as opposed to the engine code.

There are performance implications so part of the work will be validating that for most modules the level of performance achieved is acceptable.

Now I am even more confused. We were discussing V8's templates which are used to build functions and other objects. They are not a matter of ease-of-use, but a necessity for building functions and other objects with V8. Somehow I get the impression that you are thinking of C++ templates, which are an entirely different matter, having no connection to V8's templates.

Right I was thinking of C++ templates, v8's templates need to be completely separate from the API as we cannot pull in any engine specific code into the modules. In the implementation for the API provided and exported by the node.js binary, the implementation of the API can use v8 specifics but none of that can leak out to the API itself.

Doesn't everyone seem to hate C++ templates or something?

@Fishrock123 I think its a love it or hate it kind of thing. There are some people who argue strongly that using them and C++ will make the API easier to user. On the other side some people find the C style API easier to deal with.

imo, go with what is easiest to understand coming from JavaScript while remaining semantically good in most use-cases.

kkoopa · 2016-04-29T22:46:18Z

Regarding what addons to test on, don't just go for the most popular ones, pick the most complex addons, in regard to the binding part, not whatever libraries they provide. NAN's test suite, while far from comprehensive, is a good start.

There is also need to decide on what to do with Isolates. The main reason that NAN does not currently do Isolates is because of 0.10-compatibility. Should Node ever be multi-isolate, from a practical point of view? I can only think of one or two addons I know of that actually use multiple Isolates.

jasnell · 2016-04-29T22:52:10Z

00X-ABI-Stable-Module-API.md

+
+**WORK IN PROGRESS, NOT ANYWHERE NEAR COMPLETE.** 
+
+This API will be built up from a minimal set as we explore the full set of Nan functions.  To start it will only include the minimal set that we need to get started.  What this means is we have not define the full API yet but instead want to give a feel of the general shape (which is not really quite there yet). 


s/define/defined

Fishrock123 · 2016-04-30T00:21:27Z

first reactions: doesn't seem quite as minimal as discussed?

Also, ni is an obscure and pretty uninformative namespace. :/ even api might be better?

bnoordhuis · 2016-04-30T08:58:53Z

00X-ABI-Stable-Module-API.md

+* Native modules must be recompiled for each version of Node.js
+* The code within modules must often be modified for a new version of
+  Node.js
+* It.s not clear which parts of the V8 API the Node.js community believes


jasnell · 2016-04-30T16:32:27Z

@Fishrock123

first reactions: doesn't seem quite as minimal as discussed?

How so? Seems to be exactly what was discussed.

Fishrock123 · 2016-04-30T16:40:58Z

How so? Seems to be exactly what was discussed.

Perhaps it wasn't communicated fully?

I was the under the impression we'd do only:

call into JS & back
call into c++ & back
convert to basic types

As such, things like function templates seem a little beyond that? Perhaps I am wrong.

Fishrock123 · 2016-04-30T16:42:51Z

Additionally I don't think the EP process along will be enough to suffice for this. (i.e. we shouldn't just write up this conceptually and sign it off)

We should aim to not expand the EP from here directly but rather get these working in a shim, make sure that suits modules, get the feedback from that, and iterate. We should make sure to make it without any guarantees until we pass a finalized version in core though since versions of it are unlikely to be maintainable for long and the API may change.

I'm totally down to help where possible, I care about this an awful lot.

kkoopa · 2016-05-01T01:03:07Z

I'll also note that the best way of actually getting this new API into use would be to enable NAN (or a replacement) to add another (transitional) shim on top of this, ensuring backwards compatibility at least down to 0.12 or 4.0, which presumably will not have access to it. No addon maintainer wants to maintain several versions of the same code base.

jasnell · 2016-05-01T01:06:46Z

+1. But we'd have to call it naan then ;)
On Apr 30, 2016 6:03 PM, "Benjamin Byholm" [email protected] wrote:

I'll also note that the best way of actually getting this new API into use
would be to enable NAN (or a replacement) to add another (transitional)
shim on top of this, ensuring backwards compatibility at least down to 0.12
or 4.0, which presumably will not have access to it. No addon maintainer
wants to maintain several versions of the same code base.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#20 (comment)

kkoopa · 2016-05-01T01:31:17Z

One more thing, make it possible to seamlessly integrate this new (stable) API with the (unstable) VM-specific API. There is no realistic way of foreseeing every possible use case or developing a practical solution that will fit everything. What I consider one of the strong aspects of NAN is that the developer is free to pick and mix between raw V8 and NAN. Some things are not achievable in any other way, but requiring a couple of specific things here and there should not condemn the rest of the code to constant breaking. For example, promises are (currently) not handled by NAN, since they are not available in Node 0.10 and there is not realistic way of backporting all of that. However, a developer is still able to easily use promises where available, while the rest of the code can (safely) use NAN. C# has a similar concept in that mixing safe and unsafe code is quite doable.

Fishrock123 · 2016-05-01T03:22:47Z

One more thing, make it possible to seamlessly integrate this new (stable) API with the (unstable) VM-specific API. There is no realistic way of foreseeing every possible use case or developing a practical solution that will fit everything.

I think the goal will probably end up making sure at very least node core can run ontop of it. (Since that's part of VM neutrality anyways.)

jeisinger · 2016-05-02T12:06:58Z

I'm a bit surprised by the direction this took. I was under the impression that we agreed on a different plan in the F2F meeting, namely to take the NAN module and replace the remaining v8 dependencies with new wrappers, i.e., v8::Localv8::Object should become something like nan::Localnan::Object.

The direction this is now taking seems to be a newly designed C-style API.

Deviating that much from NAN will make it unnecessarily difficult for native modules to transition over. A C-style API precludes any inlining based optimizations and puts additional cognitive burden on module authors that will have to explicitly manage lifetime of handles as opposed to relying on the Local dtor deregistering them etc..

Another thing we agreed upon was that this API should insulate native modules from the VM - using the API inside node as a means of becoming fully VM neutral was an explicit non-goal, as the latter will require fundamentally different APIs.

jasnell · 2016-05-02T13:07:09Z

+1 that the primary goal was to have an API that protects module developers
from the internal VM changes. It's absolutely correct that providing a new
internal abstraction was a non-goal for this initially -- if it happens to
be usable in that way, then awesome, but if not, the focus needs to be on
making sure module developers are protected.

As far as the relationship with nan is concerned, the idea was certainly to
start with nan and iterate incrementally to begin abstracting the parts
where V8's API bleeds through.

mhdawson · 2016-05-02T15:20:04Z

@jeisinger

When we looked at the functions in Nan, they were along these lines, while the unwrapped parts of v8 were more pure C++. So in extending the Nan methods this way seemed natural. I think the first deviation in what is there that was not already that way in Nan is:

node::ni::callMethod(ni_env,recv,,,)

as these others like: Nan::New() were already there.

The other areas would be object life cyle and exception handling, but would those be more than defining objects that the same methods could be called on ? For pushScope/popScope for lifecyle the answer is probably yes, the others I'm not so sure.

In terms of using templates etc, I think that is still possible in either case, and the in-lining needs to stop before any engine code gets pulled into the module.

Agreed, that this looks more C like than when we discussed at the meeting but I think that came out of the existing methods we pulled over from Nan. We can iterate to move in the right direction.

Agree that "using the API inside node as a means of becoming fully VM neutral was an explicit non-goal for now"

@kkoopa agreed that a Nan shim to allow this to be used transparently is one of the overall goals

@Fishrock123 agreed we can't just write this up and sign off. We just want to give some visibility along this way. This eps should stay in draft while we iterate and be update frequently

jeisinger · 2016-05-02T15:30:39Z

It might be helpful to take an existing module and convert it over to the proposed API (at least parts of it to demonstrate how the new API is supposed to be used, and to get a better feeling about the changes in ergonomics this would mean).

mhdawson · 2016-05-02T22:13:00Z

@jeisinger that is what I think the next step is as well. Write the code for a minimal set of the APIs and then show it working with a simple module to make things more concrete.

mhdawson · 2016-05-02T22:13:59Z

I'll let this settle for a day or more and then incorporate the "simple" updates (typos, clarifications, etc.)

ianwjhalliday · 2016-05-03T01:44:22Z

@kkoopa Which addons are those? Sounds like they are good to have on hand for investigation once we get the balling rolling on working prototype.

There is also need to decide on what to do with Isolates. The main reason that NAN does not currently do Isolates is because of 0.10-compatibility. Should Node ever be multi-isolate, from a practical point of view? I can only think of one or two addons I know of that actually use multiple Isolates.

kkoopa · 2016-05-03T10:05:30Z

webworker-threads

Fishrock123 · 2016-05-03T19:02:44Z

Should Node ever be multi-isolate, from a practical point of view

Yes, so that workers are a thing we can do since we've been wanting to for a while but it has been too complex to review effectively. See: nodejs/node#2133

trevnorris · 2016-05-05T08:37:10Z

README.md

-The initial status shall be **"DRAFT"**.
+The initial status shall either be **"INCUBATING"** or **"DRAFT"**.
+
+If the status is **"INCUBATING"** that means that it is expected that the


Why is this necessary? The incubation period is the time spent in the PR. If the EP is still so volatile that it expects to have changes or additions over time then it shouldn't land.

If this is the initial api and later there's an addition then the addition should go in a new EP. These aren't design documents that are meant to reflect a feature's current state.

@trevnorris this mistakenly got pushed as part of an update can you comment here instead: #22

trevnorris · 2016-05-05T08:40:51Z

Please replace <PRE> with three grave symbols. (Backticks)

mhdawson · 2016-11-04T17:21:52Z

@agnat the kinds of suggestions you are making is exactly why I'd like to get this landed as a DRAFT. You could then submit PR's to make the enhancements you are suggesting and they could be discussed in the PR.

jasnell · 2016-11-04T17:28:23Z

This LGTM, we can update with follow on PRs

mhdawson · 2016-11-04T17:52:37Z

@agnat I'm going to go ahead and land the current version and then look to address your comments through PRs. Do you want to submit PRs for the suggested changes or would you like me to do that based on your comments ?

agnat · 2016-11-12T16:21:40Z

@mhdawson that is not how it is supposed to work. People spent time and effort working through this asking very valid questions in the comments above. Not only me.

Answering these questions in the comment section is not enough. Postponing them till later is not ok. These questions arise while reading the proposal. The proposal should answer them. Please respect the work reviewers put into this. Include their feedback and improve the document.

I've been following this PR since pinged on day one. I postponed the review time and again because the document is no help. Diving in anyway 17some days ago it took me more than 16 hours of work to develop a mental picture of what really is about to happen. It is the documents job to describe the system. Instead I had to work it out myself.

I'm ready to help with the document down the road, but as it stands it simply lacks the structure to do that.

The interface centric view will never be final until the thing is done. Describe the (path to a) system instead.
Stating that $X is incomplete in the document guarantees major misunderstandings. Nobody knows if you skipped a feature because you decided not to support it or if it is an oversight. Describe the scope.
Mention performance and maintenance implications.
Answer the questions above in the document.

Please provide a proper technical proposal we can work on. One we can actually discuss because it has some meat.

mhdawson · 2016-11-14T21:11:39Z

@agnat, I am going to need more insight into what you are suggesting before jumping into the work you are suggesting.

I think a higher bandwidth discussion between us would help me understand where/what you'd like changed. I'm thinking a 30 minute discussion were we go through and come up with the main headings in the doc together would go a long way in helping me understand the concrete changes that will address you concerns. Would that be possible ?

We can do that either one on one or during the weekly get together held by the group working on the PoC (Fridays at 2EST).

brodycj · 2016-11-23T17:08:27Z

Why define the C++ interface in the same place? I would rather see the C++ wrapper defined and/or implemented somewhere else. Simpler parts working together (UNIX philosophy). As an idea: could we make a new NAN version that provides the C++ wrapper?

brodycj · 2016-11-23T17:30:28Z

I would also like to see a couple more API functions/enhancements:

extern "C" equivalent to int node::Start(int argc, char *argv[])
API function to start node instance in a non-default uv_loop (with a new V8 Isolate)

1 is needed by C embedders while 2 could help enable true multithreading on Node.js.

I experimented with I would be happy to submit a new issue and maybe a PR on https://github.com/nodejs/abi-stable-node to explain what I mean by 2.

+1 for getting something landed, maybe something simple so people can start submitting PRs. What is the status? Any conclusions between @mhdawson / @agnat?

P.S. I would be happy to raise both 1 and 2 as new issue(s). 👎 for not landing a DRAFT.

Fishrock123 · 2016-12-01T20:20:34Z

Tagging with ctc-agenda as discussed in the Node Interactive NA VM Summit meeting.

jasnell · 2016-12-02T05:41:50Z

@nodejs/ctc : This EP was discussed extensively during the VM Summit today at the Node Interactive Collaborator Summit. While this is still very much a work in progress, there has been sufficient progress towards a working prototype and a well defined direction. We should make a decision about whether this EP should be accepted as a draft (and this PR landed). The recommendation of the VM Summit participants is that this should be accepted as a draft.

Trott · 2016-12-02T13:16:22Z

@nodejs/ctc Note that the CTC is being asked to review this EP, so please review it carefully if you are able.

@jasnell @Fishrock123 Any reason not to post an issue in the CTC repo and call for a vote right now? Or is there a reason the process needs to wait for the actual meeting?

jasnell · 2016-12-03T03:50:29Z

@Trott I'd like to make sure that there's at least an opportunity for the CTC members to ask any clarification questions but opening a vote is a good idea.

bnoordhuis · 2016-12-05T12:10:18Z

00X-ABI-Stable-Module-API.md

+
+struct napi_method_descriptor {
+  napi_callback callback;
+  char* utf8name;


const char*?

opened nodejs/abi-stable-node#25 to update current implementation.

bnoordhuis · 2016-12-05T12:12:14Z

00X-ABI-Stable-Module-API.md

+NODE_EXTERN napi_value napi_create_symbol(napi_env e, const char* s = NULL);
+NODE_EXTERN napi_value napi_create_function(napi_env e, napi_callback cbinfo);
+NODE_EXTERN napi_value napi_create_error(napi_env e, napi_value msg);
+NODE_EXTERN napi_value napi_create_type_error(napi_env e, napi_value msg);


How will these functions signal error? E.g. what if napi_create_object() can't create an object because the VM is out of memory or in a state where it can't be entered?

Adding error handling is the next main milestone that the time is working on.

bnoordhuis · 2016-12-05T12:15:48Z

00X-ABI-Stable-Module-API.md

+NODE_EXTERN int napi_get_string_length(napi_env e, napi_value v);
+NODE_EXTERN int napi_get_string_utf8_length(napi_env e, napi_value v);
+NODE_EXTERN int napi_get_string_utf8(napi_env e, napi_value v,
+                                     char* buf, int bufsize);


Why int instead of size_t?

opened nodejs/abi-stable-node#25 to update current implementation.

bnoordhuis · 2016-12-05T12:18:10Z

00X-ABI-Stable-Module-API.md

+
+// Methods to work with Functions
+NODE_EXTERN void napi_set_function_name(napi_env e, napi_value func,
+                                        napi_propertyname napi_value);


How would this work with an engine like spidermonkey where you would normally declare the name upfront?

Created to nodejs/abi-stable-node#26 to investigate/figure out if we need a different API here.

bnoordhuis · 2016-12-05T12:18:58Z

00X-ABI-Stable-Module-API.md

+                                  napi_value* buffer, size_t bufferlength);
+NODE_EXTERN napi_value napi_get_cb_this(napi_env e, napi_func_cb_info cbinfo);
+// V8 concept; see note in .cc file
+NODE_EXTERN napi_value napi_get_cb_holder(napi_env e, napi_func_cb_info cbinfo);


Do other engines distinguish between this and holder? (EDIT: Or is that what "V8 concept" refers to?)

Yes, the note is saying its a v8 concept.

so still a work in progress to see if it makes sense across engines, so far ok for v8 and Chakra.

bnoordhuis · 2016-12-05T12:20:05Z

00X-ABI-Stable-Module-API.md

+// Methods to support ObjectWrap
+NODE_EXTERN napi_value napi_create_constructor_for_wrap(napi_env e,
+                                                        napi_callback cb);
+NODE_EXTERN void napi_wrap(napi_env e, napi_value jsObject, void* nativeObj,


Will this work in all engines?

So far we've got it working for v8 and Chakra.

bnoordhuis · 2016-12-05T12:21:05Z

00X-ABI-Stable-Module-API.md

+// Methods to support error handling
+NODE_EXTERN void napi_throw(napi_env e, napi_value error);
+NODE_EXTERN void napi_throw_error(napi_env e, char* msg);
+NODE_EXTERN void napi_throw_type_error(napi_env e, char* msg);


const char*

opened nodejs/abi-stable-node#25 to update current implementation.

bnoordhuis · 2016-12-05T12:27:38Z

00X-ABI-Stable-Module-API.md

+typedef void (*napi_try_callback)(napi_env e, void* data);
+typedef void (*napi_catch_callback)(napi_env e, void* data);
+NODE_EXTERN bool napi_try_catch(napi_env e, napi_try_callback t,
+                                napi_catch_callback c, void* data);


I think most JS engines have an 'is exception pending?' function; V8 is the outlier with its TryCatch construct. API strawman:

// Wrapper around a v8::TryCatch. Dummy with other engines. typedef struct napi_trycatch { struct napi_trycatch *v; } napi_trycatch; NODE_EXTERN void napi_trycatch_new(napi_env e, napi_trycatch *trycatch); // Returns exception object or undefined. NODE_EXTERN napi_value napi_trycatch_exception(napi_env e, napi_trycatch trycatch); NODE_EXTERN void napi_trycatch_delete(napi_env e, napi_trycatch trycatch);

Create nodejs/abi-stable-node#27 to discuss as part of prototype

bnoordhuis · 2016-12-05T12:28:44Z

00X-ABI-Stable-Module-API.md

+However, we also believe that a C++ wrapper is possible and
+would provide ease of use. The key element is that the C++ part must all
+be inlined and only use the external functions/types exported
+by the C API.  


The problem with inlining is that there is no way to release bug fixes without everyone needing to recompile.

That is true, the sugar becomes part of the module.

mhdawson · 2016-12-05T17:27:19Z

@bnoordhuis has a number of good suggestions. I also know that the API will still change as the PoC continues so I opened issues in the abi-stable-node repo to address those.

Earlier, before we had progressed the PoC to working code it was useful to have the functions in the eps to give people a "feel" for what it might look like. At this point I'm wondering if we should remove it and replace it with a pointer to the header in the abi-stable-node repo: https://github.com/nodejs/abi-stable-node/blob/api-prototype-6.2.0/src/node_jsvmapi.h as it will continue to evolve.

jasnell · 2016-12-07T16:57:04Z

+1 to landing this EPS as a draft and iterating on from there

mhdawson · 2016-12-12T21:08:56Z

Discussed in CTC meeting last week. Agreed to land unless objects posted here. Going to land now.

First cut of the ABI-Stable-Module-API eps. It is far from complete but is intended to allow insight into the current direction/status and will be updated regularly as we progress along the investigation and can make the API definition more complete.

mhdawson · 2016-12-12T21:36:18Z

Squashed commits.

First cut of the ABI-Stable-Module-API eps. It is far from complete but is intended to allow insight into the current direction/status and will be updated regularly as we progress along the investigation and can make the API definition more complete. PR-URL: #20 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

mhdawson · 2016-12-12T21:46:04Z

Landed as 735b776

trevnorris · 2017-01-27T23:17:39Z

Just noticed that it wasn't added to the index. Added it for you in 54b199c

This was referenced Apr 29, 2016

About JS API abstract nodejs/api#17

Closed

What is the progress of the Native API？ nodejs/api#15

Closed

kkoopa reviewed Apr 29, 2016
View reviewed changes

jasnell reviewed Apr 29, 2016
View reviewed changes

bnoordhuis reviewed Apr 30, 2016
View reviewed changes

trevnorris reviewed May 5, 2016
View reviewed changes

Fishrock123 added the tsc-agenda label Dec 1, 2016

Trott mentioned this pull request Dec 5, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-12-07 nodejs/CTC#43

Closed

bnoordhuis reviewed Dec 5, 2016

View reviewed changes

This was referenced Dec 5, 2016

Tweaks to API suggested on EPS nodejs/abi-stable-node#25

Closed

Will napi_set_function_name work with spidermonkey nodejs/abi-stable-node#26

Closed

Correct abstraction for try/catch nodejs/abi-stable-node#27

Closed

joshgav mentioned this pull request Dec 5, 2016

doc: meeting notes from 2016-12-01 nodejs/abi-stable-node#23

Merged

2 tasks

Trott mentioned this pull request Dec 12, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-12-14 nodejs/CTC#46

Closed

mhdawson force-pushed the master branch from e10206f to 8888408 Compare December 12, 2016 21:34

mhdawson closed this Dec 12, 2016

mhdawson removed the tsc-agenda label Dec 12, 2016


		WORK IN PROGRESS, NOT ANYWHERE NEAR COMPLETE.

		This API will be built up from a minimal set as we explore the full set of Nan functions. To start it will only include the minimal set that we need to get started. What this means is we have not define the full API yet but instead want to give a feel of the general shape (which is not really quite there yet).

Initial version of eps for ABI-Stable-Module-API #20

Initial version of eps for ABI-Stable-Module-API #20

Conversation

mhdawson commented Apr 29, 2016

mhdawson commented Apr 29, 2016

Choose a reason for hiding this comment

bnoordhuis Apr 30, 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

kkoopa May 2, 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

kkoopa commented Apr 29, 2016

Choose a reason for hiding this comment

Fishrock123 commented Apr 30, 2016

Choose a reason for hiding this comment

jasnell commented Apr 30, 2016

Fishrock123 commented Apr 30, 2016

Fishrock123 commented Apr 30, 2016

kkoopa commented May 1, 2016

jasnell commented May 1, 2016

kkoopa commented May 1, 2016

Fishrock123 commented May 1, 2016 • edited Loading

jeisinger commented May 2, 2016

jasnell commented May 2, 2016 • edited Loading

mhdawson commented May 2, 2016

jeisinger commented May 2, 2016

mhdawson commented May 2, 2016

mhdawson commented May 2, 2016

ianwjhalliday commented May 3, 2016

kkoopa commented May 3, 2016

Fishrock123 commented May 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented May 5, 2016 • edited by Fishrock123 Loading

mhdawson commented Nov 4, 2016

jasnell commented Nov 4, 2016

mhdawson commented Nov 4, 2016

agnat commented Nov 12, 2016

mhdawson commented Nov 14, 2016

brodycj commented Nov 23, 2016

brodycj commented Nov 23, 2016 • edited Loading

Fishrock123 commented Dec 1, 2016

jasnell commented Dec 2, 2016

Trott commented Dec 2, 2016

jasnell commented Dec 3, 2016

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

mhdawson commented Dec 5, 2016 • edited Loading

jasnell commented Dec 7, 2016

mhdawson commented Dec 12, 2016

mhdawson commented Dec 12, 2016

mhdawson commented Dec 12, 2016

trevnorris commented Jan 27, 2017

bnoordhuis Apr 30, 2016 •

edited

Loading

kkoopa May 2, 2016 •

edited

Loading

Fishrock123 commented May 1, 2016 •

edited

Loading

jasnell commented May 2, 2016 •

edited

Loading

trevnorris commented May 5, 2016 •

edited by Fishrock123

Loading

brodycj commented Nov 23, 2016 •

edited

Loading

mhdawson commented Dec 5, 2016 •

edited

Loading