esm: provide named exports for builtin libs #20403

devsnek · 2018-04-29T13:43:41Z

provide named exports for all builtin libraries so that the libraries may be
imported in a nicer way for esm users: import { readFile } from 'fs'
instead of importing the entire namespace, import fs from 'fs', and
calling fs.readFile. the default export is left as the entire
namespace (module.exports)

continuation of #18131 because it got too long and github was giving unicorns

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

devsnek · 2018-04-29T13:49:07Z

@guybedford i'm not quite sure what the setter-based approach looks like (do we iterate through the exports and re-assign every property to have get/set instead of value?) how do we prevent common ways that userland monkeypatches while keeping property values updated (defineProperty) how do we make sure that deleted properties become representative in esm land

addaleax · 2018-04-29T13:52:22Z

test/fixtures/es-module-loaders/js-loader.mjs

@@ -1,10 +1,11 @@
-import _url from 'url';
+import { URL } from 'url';


Side note: This is global now, so we could just remove this line and/or split this change into a separate PR.

i left it in just in case these changes get backported to other release lines

guybedford · 2018-04-29T13:52:30Z

@devsnek yes exactly. There may well be issues with backwards compatibility for configuration/reflection or it may not provide such a performance benefit, but I think it's worth running the experiment.

guybedford · 2018-04-29T14:05:08Z

I just had a look through some common core-instrumenting packages (newrelic, old graceful-fs) and they both do a direct member expression assignment. I think we may well be ok with setter-only.

devsnek · 2018-04-29T14:15:34Z

@guybedford it is worth noting that the added time is from the iterating and creation and such at startup, not from actually calling those traps. i'm can't think of how you will build hundreds of getter/setters in less time while still having the majority of the same code for enumerable filtering and such

guybedford · 2018-04-29T14:25:10Z

@devsnek 10% startup time cost is a huge regression on its own. But I'm specifically referring to how turning core modules into proxies affects existing hot code like for (let i = 0; i < 1000) crypto.createHash('sha256').update('asdf').digest('hex').

jdalton · 2018-04-29T15:10:13Z

@guybedford The perf run mentioned was done on an incomplete implementation. None of the methods in your hot code example are wrapped (createHash, update, digest) so are unaffected.

Perf is important (duh), and at this stage it's good to keep an eye on for sure. There's plenty of perf work to be done on ES modules in Node in general (and that work will come). Since this is experimental things should be allowed to progress and take shape. Any performance concerns you have about specific JS constructions should be filed for V8 to tackle (there's plenty of runway to improve whatever needs improving).

You too could experiment with your own implementation without obstructing. If a better implementation shakes out down the road then so be it.

vsemozhetbyt · 2018-04-29T15:06:37Z

doc/api/esm.md

+
+// bar.js
+import foo from './foo.js';
+foo.one === 1; // true


Ignorable nit: as this is a file, not a REPL, maybe add assert() or console.log() here?

vsemozhetbyt · 2018-04-29T15:16:13Z

doc/api/esm.md

+
+```js
+import { readFile } from 'fs';
+readFile('./foo.txt', (err, body) => {


Nit: it seems a bit unexpected to name file content body (no body in fs doc), it seems more natural for http(s) docs. However, I can miss some conventions)

does src/source work?

Yes, it seems more neutral to me.

devsnek · 2018-04-29T15:37:40Z

CI https://ci.nodejs.org/job/node-test-pull-request/14584/

guybedford

I've done an initial review here - looking through this more closely it is looking really great actually!

Agreed a setter approach would incur a lot of property definitions that might slow us down more anyway.

@jdalton points taken that we can improve perf here over time. I ran some initial subsets of the benchmarks against this code and haven't yet spotted any issues, but it's worth keeping an eye on the hot paths here.

@devsnek I would really appreciate it if you can help me understand the wrap magic to more properly review. I'll also aim to stare at it again this week if I get a moment. I must admit the layered proxy situation also doesn't seem ideal from a perf perspective, so would be nice to be able to follow the driving use cases to see where hot paths might be carved out from that further.

guybedford · 2018-04-29T22:10:56Z

lib/internal/bootstrap/loaders.js

+    const nativeRegExp = new RegExp(`^${re}$`);
+    isNative = (fn) => {
+      if (typeof fn === 'function' &&
+          nativeRegExp.test(toString.call(fn))) {


This is quite tricky to follow, and if there are changes in v8 perhaps tricky to maintain.

Surely toString.call(fn).endsWith('{ [native code ] }') would give just as strong a guarantee?

if there are changes in v8 perhaps tricky to maintain.

just the opposite (ignoring that Function.prototype.toString is standardised now) different engines or different versions of the same engine may represent native functions differently

@guybedford For a reference implementation see this and this. The idea is to use a builtin native method as a template for detection of others. @devsnek has a todo to implement a v8 helper for this too.

Sure, perhaps a comment would help maintenance though - it's pretty tough to follow.

I'm a fan of comments for sure 📖

guybedford · 2018-04-29T22:13:12Z

lib/internal/bootstrap/loaders.js

+      if (typeof fn === 'function' &&
+          nativeRegExp.test(toString.call(fn))) {
+        const { name } = fn;
+        if (typeof name !== 'string' || !/^bound /.test(name))


name.startsWith('bound ').

any particular reason? regex is still faster at this time

👆 It's micro-opt territory here so it's up to personal preference is all.

Interesting, I didn't realise that had changed in v8. Just paying attention to the getter hot path here.

Keep in mind this a few steps/checks into the native check so many values will have already exited with false by then. This check is also performed once per changed value (so not per repeated get request).

This very much does seem on the hot path to me? get -> wrap -> isNative is on every get in the proxy, so this path seems like it will hit every access to bound functions on exports?

This startsWith check is done after the first few check so will only apply to the first access of bound functions and native functions. I don't think there's a need to get hung up on micro-optimizations at this point. If you find the need later then sure we can tweak various ways to detect xyz.

guybedford · 2018-04-29T22:44:12Z

lib/internal/bootstrap/loaders.js

+        this.exportKeys = Object.keys(this.exports);
+
+        const update = (property, value) => {
+          if (this.reflect !== undefined && this.exportKeys.includes(property))


property in this.reflect.exports might be a nicer check here.

is reflect.exports a null object?

hasOwnProperty could work too, just nice to avoid linear lookup if we can.

exportKeys could be made a Set as well

guybedford · 2018-04-29T23:08:09Z

lib/internal/bootstrap/loaders.js

+            if (typeof value === 'function')
+              value = methodUnwrapMap.get(value) || value;
+            if (ReflectSet(target, prop, value, receiver)) {
+              update(prop, ReflectGet(target, prop, receiver));


When the receiver is not equal to the target, surely we don't want to be running the update method?

For example, if someone does Object.create(module.exports).asdf = 'asdf', we don't want to be updating the ES module object.

not quite sure i understand this one, but i do know that Object.create(module.exports).asdf = 'asdf' doesn't change anything on module.exports

Exactly, so it shouldn't call the update function, whereas right now it does I believe.

@guybedford

Exactly, so it shouldn't call the update function, whereas right now it does I believe.

I cannot reproduce your concern.

import { isArray } from 'util' > Object.create(util).isArray = 'asdf' 'asdf' > isArray [Function: isArray] > util.isArray [Function: isArray]

Standard set will create a new "own" property on the receiver if it isn't the same as the target and won't update the target normally. Easiest example of this is with Object.prototype not being updated in :

x={}; // has Object.prototype ; ~= Object.create(Object.prototype) x.toString = null; assert.notEqual(x.toString, Object.prototype.toString);

This seems to be a slight bug.

@bmeck

Looks like userland can do some stuff since we allow defineProperty trapping that exhibits these behaviors,

In your example above you have removed the own property of fs when you assigned it fs.readFile=null because in the setter it performed delete fs.readFile. So when you logged both fs.readFile values they had a fs.hasOwnProperty("readFile") of false and are logging the Object.prototype.readFile value. The deleteProperty trap should have updated the ns values to be undefined though, so I should check that out.

In my own implementation I get the expected results:

fs 123 from namespace undefined fs 456 from namespace undefined

@bmeck

In my local tests for the worstTargetEver things are looking alright.

we need to add a test to ensure we don't get value since that is an own property on the receiver so we can prevent regressions.

This looks good

import { foo, bar } from "worstTargetEver" import w from "worstTargetEver" w.foo = 3 foo // undefined w.foo // undefined bar // 3 w.bar // 3

right now it could update to Object.prototype.foo since there isn't a descriptor anymore but we still do a non-guarded Get after the Set. I didn't see any existing things in core that would be problematic.

The worstTargetEver example did not result in a value getting assigned to Object.prototype.foo

Reflect.has(Object.prototype, "foo") // false

This could be because this line of worstTargetEver...

Object.defineProperty(this, 'foo', {value}) // not on target, need to ensure it isn't picked up

...the this, while not the target, is the proxy, so it's ok.

this somewhat shows a problem if setters are defining other properties that might need to be updated.

The example above works as expected, to me at least.

I'll work with @devsnek to add tests and ensure the PR results align with my local.

@bmeck I had the same concern at the beginning, but because of the limited scope of this PR (only applies to core modules) I didn't think we have to be that watertight, because we don't really have those weird setters/getters.

@jdalton which example? we have multiple in this thread.

@TimothyGu since userland is able to create those situations by mutating core I think we still need to at least document how things work. I don't find the synchronization mismatch intuitive, but am not trying to stop this PR as long as it is well understood.

@bmeck @TimothyGu As mentioned above (here and here) there should be no synchronization mismatch or unexpected gotchas once I get a chance to update this PR (I'll add tests too).

guybedford · 2018-04-29T23:09:52Z

lib/internal/bootstrap/loaders.js

+        const methodWrapMap = new WeakMap();
+        const methodUnwrapMap = new WeakMap();
+
+        const wrap = (target, name, value) => {


Can you provide a few examples for what this is mitigating? Would help a lot to follow this, and a comment would likely help as well.

👆 There should be unit tests for it (or added if not) but the gist of the issue is that native methods (so not plain or bound ones) may be picky about their this binding and need wrapping for general use when separated from their object.

process.cwd.call(new Proxy(process, {})) // TypeError: Illegal invocation

We should totally file a v8 bug on this though since this doesn't affect NodeChakra.
Once fixed the workaround isn't needed.

Update: V8 bug here.

Amazing, that helps a lot to know! If it seems like it may well be removed by the time this is unflagged, then we really don't need to worry about the perf on this path for now.

Could be worth noting the justification and v8 issue in a comment.

I'm 👍 on adding comments. I'll add moar to my implementation too.

Update:

Once the V8 bug is fixed in addition to dropping the proxy wrapper for native methods we can also conditionally exclude the get trap on the module.exports proxy for exports that are functions or those we expect to be [object Object] (so likely all builtin modules). Since then, the only thing the get trap will be used for is correcting toStringTag values.

guybedford · 2018-04-29T23:11:33Z

lib/internal/bootstrap/loaders.js

+                typeof value !== 'string') {
+              const toStringTag = ObjectToString.call(target).slice(8, -1);
+              return toStringTag === 'Object' ? value : toStringTag;
+            }


guybedford · 2018-04-29T23:12:46Z

lib/internal/bootstrap/loaders.js

+              const toStringTag = ObjectToString.call(target).slice(8, -1);
+              return toStringTag === 'Object' ? value : toStringTag;
+            }
+            return wrap(target, prop, value);


Perhaps do the function check here so we don't need to go into the wrap function unnecessarily as this is our hot path.

guybedford · 2018-04-29T23:13:11Z

lib/internal/bootstrap/loaders.js

+            return descriptor;
+          },
+          get: (target, prop, receiver) => {
+            const value = ReflectGet(target, prop, receiver);


I wonder which is faster - target[prop] or ReflectGet(target, prop)? Worth thinking of this one from a perf perspective.

doesn't matter, because i have to pass the receiver through

Again, I'm not sure we want to support prototypal getters here? Could they not be a shortpath:

get (target, prop, receiver) { if (target !== receiver) return ReflectGet(target, prop, receiver); // ... target receiver case }

The get trap here is just to detect a native method that needs wrapping and handle the toString proxy juggle. There is no other editorial bits going on here.

guybedford · 2018-04-29T23:14:46Z

lib/internal/bootstrap/loaders.js

+            }
+            return wrap(target, prop, value);
+          },
+          __proto__: null,


While it's good to be safe, I'm not sure this is a necessary protection here?

👆 It's a precaution since trap handlers, like descriptors, will read inherited traps/descriptors as well as own.
Does Node have a convention established for this?

places needing to avoid Object.prototype use Object.create in the existing codebase but loader has some usage of __proto__ in literals. No linting or style guide rules exist for Node to my knowledge on this topic.

guybedford · 2018-04-29T23:17:15Z

lib/internal/modules/esm/translators.js

+  // slice 'node:' scheme
+  const id = url.slice(5);
+  NativeModule.require(id);
+  const module = NativeModule.getCached(id);


const module = NativeModule.require(id) should work here and avoid a double lookup :)

getCached returns the entire module object, require just gives the exports object

Ah right, I see, we can reconsider the interface if necessary then for future work.

devsnek · 2018-04-30T00:50:59Z

interesting thought i should have had a while ago... i should be able to ignore this entire path for all the internal modules right? (conditionally on --expose-internals)

jdalton · 2018-04-30T01:05:48Z

interesting thought i should have had a while ago... i should be able to ignore this entire path for all the internal modules right? (conditionally on --expose-internals)

Yep. I didn't even know --expose-internals was a thing.

devsnek · 2018-04-30T01:26:37Z

that should really improve that degraded startup time 😅

devsnek · 2018-05-01T12:47:48Z

CI https://ci.nodejs.org/job/node-test-pull-request/14612/

bmeck · 2018-05-01T13:21:54Z

lib/internal/bootstrap/loaders.js

+            if (typeof value === 'function')
+              value = methodUnwrapMap.get(value) || value;
+            if (ReflectSet(target, prop, value, receiver)) {
+              update(prop, ReflectGet(target, prop, receiver));


Standard set will create a new "own" property on the receiver if it isn't the same as the target and won't update the target normally. Easiest example of this is with Object.prototype not being updated in :

x={}; // has Object.prototype ; ~= Object.create(Object.prototype) x.toString = null; assert.notEqual(x.toString, Object.prototype.toString);

This seems to be a slight bug.

bmeck · 2018-05-01T16:45:14Z

lib/internal/bootstrap/loaders.js

+              update(prop, ReflectGet(target, prop));
+              return true;
+            }
+            return false;


this line gets hit when you do:

import events from 'events'; Object.defineProperty(events, 'defaultMaxListeners', {value: 333}) /* TypeError: 'defineProperty' on proxy: trap returned falsish for property 'defaultMaxListeners' at Function.defineProperty (<anonymous>) at file:///Users/bfarias/Documents/node/t.mjs:2:8 at ModuleJob.run (internal/modules/esm/module_job.js:106:14) */

But it should probably report this as an error for redefining like it does right now:

> Object.defineProperty(events, 'defaultMaxListeners', {value: 333}) TypeError: Cannot redefine property: defaultMaxListeners at Function.defineProperty (<anonymous>)

@devsnek @bmeck
By changing the defineProperty trap from

if (ReflectDefineProperty(target, prop, descriptor)) { update(prop, ReflectGet(target, prop)); return true; } return false;

to

ObjectDefineProperty(target, prop, descriptor); update(prop, ReflectGet(target, prop)); return true;

we get the expected error:

TypeError: Cannot redefine property: defaultMaxListeners at Function.defineProperty (<anonymous>)

TimothyGu · 2018-05-09T16:19:43Z

lib/internal/bootstrap/loaders.js

@@ -193,6 +206,12 @@
    '\n});'
  ];

+  const getOwn = (target, property, receiver) => {
+    return ObjectHasOwnProperty.call(target, property) ?


Use Reflect.apply instead.

TimothyGu · 2018-05-09T16:23:53Z

test/es-module/test-esm-live-binding.mjs

+
+if (threw) {
+  throw error;
+}


Use finally block instead. Just curious, why is it necessary to reset the property even in case of thrown exception?

I wasn't sure how the runner operates. If it loads tests and runs them in sequence in a shared process then even if there are failed tests augmented environments could effect other tests if not properly reset/cleaned.

@jdalton Each test is in a separate process. The Python test runner spawns node processes in parallel.

guybedford

This has cleaned up really nicely! Will be great to see it moving forward. Note I see we're getting the fs promises warnings when loading fs now.

jdalton · 2018-05-09T21:58:29Z

This has cleaned up really nicely! Will be great to see it moving forward. Note I see we're getting the fs promises warnings when loading fs now.

Thanks! Yap, I mentioned that to Gus this morning. That will be resolved by #20632.

Notable Changes: * **addons**: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668) * **assert**: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485) * **crypto**: - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039) * **http**: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611) * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639) - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377) - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541) * **esm**: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403) * **timers**: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298) PR-URL: #20724

Notable Changes: * **addons**: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668) * **assert**: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485) * **crypto**: - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039) * **http**: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611) * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) [#20639](#20639) - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377) - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541) * **esm**: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403) * **timers**: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298) PR-URL: #20724

* addons: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) #20668 * assert: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) #20485 * crypto: - The `authTagLength` option has been made more flexible (Tobias Nießen) #20235) #20039 * esm: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) #20403 * http: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) #20075 #20611 * module: - add --preserve-symlinks-main (David Goldstein) #19911 * timers: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) #20298 * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) #20639 - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) #19377 - Support for multi-threaded embedding has been improved. (Anna Henningsen) #20542 #20539 #20541 PR-URL: #20724

devsnek added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Apr 29, 2018

devsnek requested review from bmeck, jkrems and guybedford April 29, 2018 13:43

addaleax reviewed Apr 29, 2018

View reviewed changes

vsemozhetbyt reviewed Apr 29, 2018

View reviewed changes

devsnek force-pushed the esm-builtin-module-namespaces branch from 56fe149 to d596353 Compare April 29, 2018 18:17

guybedford reviewed Apr 29, 2018

View reviewed changes

bmeck suggested changes May 1, 2018

View reviewed changes

bmeck reviewed May 1, 2018

View reviewed changes

devsnek force-pushed the esm-builtin-module-namespaces branch 2 times, most recently from a084f53 to d81d44d Compare May 2, 2018 15:28

bmeck mentioned this pull request May 3, 2018

ESM modules and json importing [feature request] #20494

Closed

devsnek force-pushed the esm-builtin-module-namespaces branch from d81d44d to 6022738 Compare May 9, 2018 14:57

TimothyGu reviewed May 9, 2018

View reviewed changes

guybedford approved these changes May 9, 2018

View reviewed changes

devsnek force-pushed the esm-builtin-module-namespaces branch from de87636 to 5df014e Compare May 9, 2018 23:28

addaleax mentioned this pull request May 14, 2018

v10.2.0 proposal #20724

Merged

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label May 15, 2018

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label May 15, 2018

bmeck mentioned this pull request May 17, 2018

Pluggable Loaders to support multiple use cases nodejs/modules#82

Closed

bmeck mentioned this pull request May 23, 2018

Feature: Mock modules (injection) nodejs/modules#98

Closed

chrisveness mentioned this pull request May 25, 2018

fs: move fs/promises to fs.promises #20504

Merged

4 tasks

ofrobots mentioned this pull request Jun 26, 2018

ExperimentalWarning: The fs.promises API is experimental googleapis/google-auth-library-nodejs#392

Closed

TimothyGu mentioned this pull request Jul 25, 2018

.mjs fails webpack build node-fetch/node-fetch#493

Closed

davidchambers mentioned this pull request Apr 29, 2021

update Node.js version range davidchambers/doctest#131

Merged

		@@ -1,10 +1,11 @@
		import _url from 'url';
		import { URL } from 'url';

esm: provide named exports for builtin libs #20403

esm: provide named exports for builtin libs #20403

Conversation

devsnek commented Apr 29, 2018

Checklist

devsnek commented Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Apr 29, 2018

guybedford commented Apr 29, 2018

devsnek commented Apr 29, 2018

guybedford commented Apr 29, 2018

jdalton commented Apr 29, 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

devsnek commented Apr 29, 2018

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

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

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

jdalton Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton May 2, 2018 • edited Loading

Choose a reason for hiding this comment

jdalton May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

devsnek commented Apr 30, 2018 • edited Loading

jdalton commented Apr 30, 2018

devsnek commented Apr 30, 2018

devsnek commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton May 1, 2018 • edited Loading

Choose a reason for hiding this comment

devsnek commented Apr 29, 2018 •

edited

Loading

jdalton commented Apr 29, 2018 •

edited

Loading

devsnek Apr 29, 2018 •

edited

Loading

jdalton Apr 30, 2018 •

edited

Loading

jdalton Apr 30, 2018 •

edited

Loading

jdalton Apr 30, 2018 •

edited

Loading

jdalton May 2, 2018 •

edited

Loading

jdalton May 2, 2018 •

edited

Loading

jdalton May 2, 2018 •

edited

Loading

jdalton Apr 30, 2018 •

edited

Loading

jdalton Apr 30, 2018 •

edited

Loading

jdalton Apr 30, 2018 •

edited

Loading

devsnek commented Apr 30, 2018 •

edited

Loading

jdalton May 1, 2018 •

edited

Loading

jdalton commented May 9, 2018 •

edited

Loading