New AMD-style module loader. #254

aomarks · 2018-04-26T19:44:52Z

This is a replacement for our use of RequireJS to use as the loader for our ES modules transform. It is AMD-ish, but not 100% compliant with the AMD spec or RequireJS.

Differences from RequireJS:

0.9 KB minified + compressed vs 6.6 KB for RequireJS.
Correctly resolves relative paths above the base URL (see [build/polyserve] AMD transform/requirejs results in multiple requests for same import #211 and possibly [polyserve] Duplicate CE definition errors in Firefox and Edge when combining paths and bare specifiers in tests #203).
Is entirely path-based. Does not allow registering a module by name. The only API is define(deps, factory).
define() module factories are always executed. RequireJS only starts executing a module dependency graph when triggered with a require(). The Babel AMD transform only emits define() calls, so this eliminates the need for us to inject a require() call during build.
Identifies top-level scripts and ensures they execute in order, like type=module scripts do. This would eliminate some complexity in our build pipeline, which does the same thing using code generation, and has to do some ugly tricks to preserve script-order information through the HtmlSplitter process.
Modules can request a meta dependency, which is resolved to {url: <moduleUrl>}. This should be able to simplify our build-time import.meta transform, and has the advantage of not being tied to a particular static path (i.e. you could move the module around your tree post-build and still get a correct URL).
Modules always provide an exports object to their dependents, even if nothing was exported from the module, which is more similar to native ES modules. This would fix https://github.com/Polymer/polymer-cli/issues/995 in the general case.

bicknellr

(just talked offline) It sounds like this needs to work in at least one place that doesn't have URL (IE11), but here's a few things that it could be useful for anyway.

bicknellr · 2018-04-26T20:33:44Z

packages/amd-loader/amd-loader.js

+Module.prototype.resolveUrl = function(path) {
+  if (path.indexOf('://') !== -1) {
+    // Already a fully qualified URL.
+    // TODO(aomarks) Is this a good enough check?


If you have URL, then I think you can try-catch new URL(path), which throws if it's not given a full URL, IIRC.

bicknellr · 2018-04-26T20:38:54Z

packages/amd-loader/amd-loader.js

+    return path;
+  }
+  // Just let the browser do path resolution/normalization.
+  Module.resolverAnchor.href = this.basePath + path;


Also, if you have URL, (new URL(path, this.basePath)).href is safer since it handles joining them properly:

> "http://example.com/page" + "some/path.js" < "http://example.com/pagesome/path.js"

... when really it should be:

> (new URL("some/path.js", "http://example.com/page")).href < "http://example.com/some/path.js"

(this.basePath needs to be a full URL though.)

bicknellr · 2018-04-26T20:49:03Z

packages/amd-loader/amd-loader.js

+    return url + '/';
+  }
+  // http://example.com/foo/bar.html -> http://example.com/foo/
+  return url.substring(0, lastSlash + 1);


Also, if you have URL: const getBasePath = url => new URL("./", url).href;

Worth noting that IE 11 does not have URL, but the webcomponents polyfill includes it.

rictic · 2018-04-26T22:38:31Z

packages/amd-loader/README.md

+
+# @polymer/amd-loader
+
+A JavaScript library that loads AMD-style modules.


Let's add some text saying that it loads path style modules and not name style modules, and that it's intended to mimic the semantics of es6 modules

Yes, let's call out that it's a subset and mention the differences you have in the PR description.

Just pushed a much more complete README, PTAL.

rictic · 2018-04-26T22:41:04Z

packages/amd-loader/amd-loader.js

+(function() {
+
+function Registry() {
+  this.modules = {};


Use Object.create(null) for a map-style object, otherwise stuff like this.modules['valueOf'] will be truthy.

rictic · 2018-04-26T22:41:50Z

packages/amd-loader/amd-loader.js

+  this.modules = {};
+  this.topLevelIdx = 0;
+  this.previousTopLevelUrl = undefined;
+  this.pendingDefine = undefined;


Document these fields please. Maybe use // @ts-check and jsdoc to get basic type checking, or use full typescript and tsc can also handle transpiling to ES5

+1 on TypeScript

Converted to TypeScript, added tslint, and added a fair bit of documentation. PTAL!

rictic · 2018-04-26T22:43:10Z

packages/amd-loader/amd-loader.js

+function getBasePath(url) {
+  // TODO(aomarks) Maybe a better way to do this.
+  var lastSlash = url.lastIndexOf('/');
+  if (lastSlash === url.indexOf('://') + 2) {


What about urls like //example.com?

This should be handled now. All URLs are now normalized using the <a> before we compute the base path, which should always add an explicit scheme. Tested it in Chrome/Firefox/Safari/IE11 to be sure.

rictic · 2018-04-26T22:43:46Z

packages/amd-loader/amd-loader.js

+    return url + '/';
+  }
+  // http://example.com/foo/bar.html -> http://example.com/foo/
+  return url.substring(0, lastSlash + 1);


Worth noting that IE 11 does not have URL, but the webcomponents polyfill includes it.

rictic · 2018-04-26T22:46:52Z

packages/amd-loader/amd-loader.js

+  this.pendingDefine = undefined;
+}
+
+Registry.prototype.get = function(url) {


Document these methods as well please. For most of them, just the types of the params and return is enough, but for ones with complicated invariants they're maintaining those would be valuable as well.

justinfagnani · 2018-04-26T22:58:56Z

A few high-level things:

@azakus apparently added the URL polyfill to the platform layer of the webcomponents polyfills, so it should be ok to use it.
If we consider this to be more of a polyfill for modules, I wonder if it should be loaded as part of the webcomponents polyfills? <- @sorvell @kevinpschaaf
It'd be nice to write this in TypeScript to get the type-checking benefits we're used to. TODO'able since we want to not spend too much time on this now.

aomarks · 2018-04-26T23:09:24Z

@azakus apparently added the URL polyfill to the platform layer of the webcomponents polyfills, so it should be ok to use it.

Seems like with some unit tests we can get by with a built-in function that just has to find a base path from a URL, no? The URL polyfill is bigger than this entire library, and in theory this loader could be useful to people who don't otherwise need any webcomponents polyfills.

justinfagnani · 2018-04-26T23:01:10Z

packages/amd-loader/README.md

+
+# @polymer/amd-loader
+
+A JavaScript library that loads AMD-style modules.


Yes, let's call out that it's a subset and mention the differences you have in the PR description.

justinfagnani · 2018-04-26T23:01:57Z

packages/amd-loader/amd-loader.js

+
+(function() {
+
+function Registry() {


Does Registry need to be a "class" at all? If it's a singleton you can just put the state and functions in the module closure.

Done. I thought the class might be easier to read, but this seems simpler. Incidentally it helps with minimization, since neither babel-minify nor uglify-js seem to want to rename prototype properties.

Also got rid of the Module class, from offline discussion. Size is now down to 0.7KB :)

justinfagnani · 2018-04-26T23:02:47Z

packages/amd-loader/amd-loader.js

+  this.modules = {};
+  this.topLevelIdx = 0;
+  this.previousTopLevelUrl = undefined;
+  this.pendingDefine = undefined;


+1 on TypeScript

justinfagnani · 2018-04-26T23:03:46Z

packages/amd-loader/amd-loader.js

+
+(function() {
+
+function Registry() {


Even though what's inside the module closure is top-level, I'd still indent it. Initially I thought some things below were outside of the IIFE for some reason.

This is just what clang-format does. I think I prefer it?

justinfagnani · 2018-04-26T23:19:01Z

packages/amd-loader/amd-loader.js

+    }
+  }
+
+  for (var i = 0; i < (deps || []).length; i++) {


(deps || []).length will be evaluated every iteration

justinfagnani · 2018-04-26T23:23:39Z

packages/amd-loader/amd-loader.js

+    } else if (depSpec === 'require') {
+      factoryArgs[i] = this.makeDynamicImporter();
+
+    } else if (depSpec === 'meta') {


AMD includes a "module" built-in dependency, but the spec doesn't seem to specify it that I can see. In IMD it has the exports and id properties. Maybe we dug that out of RequireJS source. Seems like in this loader module.id might === import.meta.url.

Looks like the AMD spec defers to the CommonJS spec for module, which says:

The "module" object must have a "id" property that is the top-level "id" of the module. The "id" property must be such that require(module.id) will return the exports object from which the module.id originated. (That is to say module.id can be passed to another module, and requiring that must return the original module). When feasible this property should be read-only, don't delete.

The "module" object may have a "uri" String that is the fully-qualified URI to the resource from which the module was created. The "uri" property must not exist in a sandbox.

http://wiki.commonjs.org/wiki/Modules/1.1.1#Module_Context

I'm open to using module.id or module.uri (or both). Although, I do also like the idea of providing it via meta, because we are trying to match the semantics of import.meta. I don't know if import.meta.scriptElement is going to happen, but if it did, then we would have a natural place to put it. I think this discussion depends on what our position is on interopability with other AMD modules/loaders.

justinfagnani · 2018-04-26T23:25:56Z

packages/amd-loader/amd-loader.js

+  };
+
+  script.onerror = function() {
+    throw new Error('error loading module', url);


does this need to propagate to dependents?

Yes, and this now happens.

justinfagnani · 2018-04-26T23:27:31Z

packages/amd-loader/amd-loader.js

+
+function getBasePath(url) {
+  // TODO(aomarks) Maybe a better way to do this.
+  var lastSlash = url.lastIndexOf('/');


This might have issues with slashes in query parameters or fragments. They are possible in module specifiers.

Added handling for those cases. Will cover in unit tests.

justinfagnani · 2018-04-26T23:29:29Z

packages/amd-loader/package.json

@@ -0,0 +1,16 @@
+{
+  "name": "@polymer/amd-loader",


We might want a name that indicates that this isn't full AMD: amd-path-loader?

aomarks

Converted to TypeScript, addressed some comments (still some pending), and did a little refactoring (which incidentally dropped the size a bit too). Also added minified output and tslint.

aomarks · 2018-04-27T00:44:18Z

packages/amd-loader/amd-loader.js

+
+(function() {
+
+function Registry() {


This is just what clang-format does. I think I prefer it?

aomarks · 2018-04-27T04:47:21Z

packages/amd-loader/amd-loader.js

+
+(function() {
+
+function Registry() {


Done. I thought the class might be easier to read, but this seems simpler. Incidentally it helps with minimization, since neither babel-minify nor uglify-js seem to want to rename prototype properties.

aomarks · 2018-04-27T04:47:43Z

packages/amd-loader/amd-loader.js

+(function() {
+
+function Registry() {
+  this.modules = {};


aomarks · 2018-04-27T04:48:05Z

packages/amd-loader/amd-loader.js

+  this.modules = {};
+  this.topLevelIdx = 0;
+  this.previousTopLevelUrl = undefined;
+  this.pendingDefine = undefined;


Converted to TypeScript, added tslint, and added a fair bit of documentation. PTAL!

aomarks · 2018-04-27T04:48:10Z

packages/amd-loader/amd-loader.js

+  this.pendingDefine = undefined;
+}
+
+Registry.prototype.get = function(url) {


aomarks · 2018-04-27T04:51:21Z

packages/amd-loader/amd-loader.js

+function getBasePath(url) {
+  // TODO(aomarks) Maybe a better way to do this.
+  var lastSlash = url.lastIndexOf('/');
+  if (lastSlash === url.indexOf('://') + 2) {


This should be handled now. All URLs are now normalized using the <a> before we compute the base path, which should always add an explicit scheme. Tested it in Chrome/Firefox/Safari/IE11 to be sure.

aomarks · 2018-04-27T04:51:34Z

packages/amd-loader/amd-loader.js

+    }
+  }
+
+  for (var i = 0; i < (deps || []).length; i++) {


aomarks · 2018-04-27T05:01:32Z

packages/amd-loader/amd-loader.js

+
+function getBasePath(url) {
+  // TODO(aomarks) Maybe a better way to do this.
+  var lastSlash = url.lastIndexOf('/');


Added handling for those cases. Will cover in unit tests.

aomarks · 2018-04-27T05:08:12Z

packages/amd-loader/amd-loader.js

+    } else if (depSpec === 'require') {
+      factoryArgs[i] = this.makeDynamicImporter();
+
+    } else if (depSpec === 'meta') {


Looks like the AMD spec defers to the CommonJS spec for module, which says:

The "module" object must have a "id" property that is the top-level "id" of the module. The "id" property must be such that require(module.id) will return the exports object from which the module.id originated. (That is to say module.id can be passed to another module, and requiring that must return the original module). When feasible this property should be read-only, don't delete.

The "module" object may have a "uri" String that is the fully-qualified URI to the resource from which the module was created. The "uri" property must not exist in a sandbox.

http://wiki.commonjs.org/wiki/Modules/1.1.1#Module_Context

I'm open to using module.id or module.uri (or both). Although, I do also like the idea of providing it via meta, because we are trying to match the semantics of import.meta. I don't know if import.meta.scriptElement is going to happen, but if it did, then we would have a natural place to put it. I think this discussion depends on what our position is on interopability with other AMD modules/loaders.

justinfagnani · 2018-04-27T16:19:10Z

packages/amd-loader/src/amd-loader.ts

+        factory.apply(null, args);
+      }
+      mod.resolved = true;
+      for (const callback of mod.notify) {


I'd be careful with for/of loops if we're trying to be small here, or at least explicitly set downlevelIteration to false in tsconfig.

Added downlevelIteration: false.

justinfagnani · 2018-04-27T16:20:01Z

packages/amd-loader/src/amd-loader.ts

+   * that Module objects are created and registered before they are loaded,
+   * which is why this is not simply part of construction.
+   */
+  define(deps: string[], factory?: (...args: {}[]) => void) {


Does Babel never output factories that return a value?

We think not.

justinfagnani · 2018-04-27T16:44:58Z

packages/amd-loader/src/amd-loader.ts

+    if (this.needsLoad === false) {
+      return;
+    }
+    this.needsLoad = true;


justinfagnani · 2018-04-27T17:03:44Z

packages/amd-loader/src/amd-loader.ts

+    const mod = this;
+    script.onload = function() {
+      if (pendingDefine !== undefined) {
+        pendingDefine(mod);


I think you could just create the module object in define() and stash it in a global, and then you know if define was called. If it wasn't, then create one. This should let you get rid of pendingDefine.

Discussed offline. The timing doesn't work out -- the module object has already been created by the parent module at this time (whichever one first declared a dependency on this module), and here we are actually waiting to find out our URL so that we can associate that existing module object with the dependencies and factory that we now have.

justinfagnani · 2018-04-27T17:05:38Z

packages/amd-loader/src/amd-loader.ts

+        args[i] = this.exports;
+
+      } else if (depSpec === 'require') {
+        args[i] = this.require.bind(this);


Is this right? I think the require that's injected accepts a single string argument? This might need to be:

args[i] = (dep) => this.require([dep]);

Yep, dynamic require takes an array of dependencies.

justinfagnani · 2018-04-27T17:06:05Z

packages/amd-loader/src/amd-loader.ts

+      } else if (depSpec === 'require') {
+        args[i] = this.require.bind(this);
+
+      } else if (depSpec === 'meta') {


I'd drop this for now pending a change to the import.meta transform.

Going with meta for now.

justinfagnani · 2018-04-27T17:08:14Z

packages/amd-loader/src/amd-loader.ts

+ * it doesn't exist yet.
+ */
+function getModule(url: string): Module {
+  let mod = registry[url];


If you want to be really safe against someone using a module name like __proto__, add a prefix to the url.

We know that the keys are all fully qualified URLs (now with a branded type to be really clear), so I don't think this is a problem.

justinfagnani · 2018-04-27T17:11:52Z

packages/amd-loader/src/amd-loader.ts

+  private resolved = false;
+
+  /** Callbacks from dependents waiting for this module to resolve. */
+  private notify: Array<() => void> = [];


Rather than an array of callbacks, keep an array of dependent modules, and inline onDepResolved into the loop where the callbacks are called.

Discussed offline. The current factoring exploits the fact that the same implementation of require can be used both for module initialization ("define"), and as the function we pass to modules requesting a dynamic "require". However, in the latter case, we don't have any dependents to notify, so that happens only in "define" with an additional closure.

usergenic · 2018-04-27T19:08:12Z

packages/amd-loader/.gitignore

@@ -0,0 +1,2 @@
+/node_modules/
+/lib/


If you're going to have a .gitignore, you're going to need a .npmignore because, if memory serves, npm publish honors a .gitignore if no .npmignore is present, because the world would be too predictable and reasonable otherwise.

Yep. Empty .npmignore will do

usergenic · 2018-04-27T19:42:32Z

packages/amd-loader/src/amd-loader.ts

+    }
+  }
+
+  for (let i = 0; i < deps.length; i++) {


Maybe this comment is golfy, but is there a reason not to do:

for (const depSpec of deps) { ... args.push(...) ... }

instead of args[i] = ...?

usergenic · 2018-04-27T19:51:00Z

packages/amd-loader/src/amd-loader.ts

+  };
+
+  script.onerror = () => {
+    throw new Error('error loading module ' + mod.url);


Does this do anything functionally different than if you hadn't added the onerror?

No, but now it does because there is propagation.

usergenic · 2018-04-27T19:53:27Z

packages/amd-loader/src/amd-loader.ts

+  checkDeps();
+}
+
+let pendingDefine: ((mod: Module) => void)|undefined = undefined;


This is kindof maybe explained inside window.define's leading comment, but it could probably use a comment of its own to better annotate its role in the process and/or why it is a global singleton?

I rearranged the window.define function to the top so that it is closer to this variable.

usergenic · 2018-04-27T20:10:06Z

packages/amd-loader/src/amd-loader.ts

+  // fired by this <script>, which will be handled by the loading script,
+  // which will invoke the callback with our module object.
+  let defined = false;
+  pendingDefine = (mod) => {


Talked about this offline, but I was concerned here that this was overwriting a global singleton and that doing so invites, potentially, a tricky-to-diagnose error condition if there is a possibility of two consecutive define() invocations before the triggering of the pendingDefine() inside script.onload.

aomarks · 2018-04-28T06:53:47Z

I just pushed some WCT tests, and some bug fixes that they revealed. PTAL!

The major remaining piece here is error handling, e.g. it doesn't yet support invoking the error callback for dynamic require calls when a dependency cannot be resolved.

justinfagnani · 2018-04-28T16:49:30Z

packages/amd-loader/.gitignore

@@ -0,0 +1,2 @@
+/node_modules/
+/lib/


Yep. Empty .npmignore will do

justinfagnani · 2018-04-28T16:49:57Z

packages/amd-loader/src/amd-loader.ts

@@ -0,0 +1,237 @@
+type modCallback = (...args: {}[]) => void;


justinfagnani · 2018-04-28T16:50:36Z

packages/amd-loader/src/amd-loader.ts

+  return mod;
+}
+
+type normalizedUrl = string&{_normalized: never};


capitalize and move to top of file

justinfagnani · 2018-04-28T16:51:46Z

packages/amd-loader/src/amd-loader.ts

+  define: (deps: string[], factory: modCallback) => void;
+}
+
+(function() {


you shouldn't need an IIFE in the source - either the module setting in tsconfig will create one, or just use a block, which should be compiled to an IIFE.

Actually as far as I can tell, TypeScript will not generate an IIFE even when the target is es5, so it will leak transpiled let and const variables up to function/global scope. (It does at least rename variables to prevent collisions within the compiled program.) There also seems to be no module setting that changes this behavior (anyway, this file is not a module).

The default module setting is CommonJS, which might not be obvious in the output here because you have no imports or exports. "module": "none" and using a block might do it.

Nope, module: none and target:es5 with a block will just emit a block with vars:

tsconfig.json

{ "compilerOptions": { "target": "es5", "module": "none" } }

test.ts

{ let x = 0; }

test.js

{ var x = 0; }

Wow, that just seems broken... unless the rationale is without a module setting they can't know that this file isn't the whole program?

Ok, then. Thanks for humoring me! 🤷‍♂️

justinfagnani · 2018-04-28T16:55:01Z

packages/amd-loader/src/amd-loader.ts

+ *  - http://example.com/foo/?qu/ery#fr/ag => http://example.com/foo/
+ */
+function getUrlBase(url: normalizedUrl): normalizedUrl {
+  url = url.split('?')[0] as normalizedUrl;


can you use the anchor for this?

.pathname could be useful, but then we need to reconstruct scheme, host, etc., so we think this is simpler for now.

justinfagnani · 2018-04-28T23:50:48Z

Do you have an idea for a new name that better hints at the specific functionality?

aomarks · 2018-04-29T00:03:54Z

Do you have an idea for a new name that better hints at the specific functionality?

Does seem like we need something, but nothing so far sounds great:

amd-path-loader
tiny-amd-loader
require-less
amd-loader-lite
es-module-amd-loader

Any other ideas?

aomarks · 2018-04-29T00:26:52Z

Yep. Empty .npmignore will do

Done. Needed some more stuff in .npmignore anyway.

justinfagnani · 2018-04-29T00:48:14Z

amd-path-loader isn't bad, IMO. js-module-loader?

aomarks · 2018-05-01T01:21:37Z

amd-path-loader isn't bad, IMO. js-module-loader?

We decided to go with esm-amd-loader because the key distinguishing feature is that it replicates ES module behavior.

Disable AMD loader tests on Windows because WCT is broken.

aomarks requested review from rictic, dfreedm, sorvell, justinfagnani and kevinpschaaf April 26, 2018 19:44

googlebot added the cla: yes label Apr 26, 2018

aomarks force-pushed the amd-loader branch from 2cb6c72 to d1244dd Compare April 26, 2018 19:59

bicknellr reviewed Apr 26, 2018

View reviewed changes

rictic reviewed Apr 26, 2018

View reviewed changes

justinfagnani reviewed Apr 26, 2018

View reviewed changes

aomarks commented Apr 27, 2018

View reviewed changes

justinfagnani reviewed Apr 27, 2018

View reviewed changes

aomarks requested a review from usergenic April 27, 2018 18:51

usergenic reviewed Apr 27, 2018

View reviewed changes

justinfagnani reviewed Apr 28, 2018

View reviewed changes

justinfagnani approved these changes May 1, 2018

View reviewed changes

aomarks added 3 commits April 30, 2018 18:16

Initial commit of a minimal AMD-style loader for Polymer build.

2042281

Some refactoring.

b61a884

Convert to TypeScript.

d12dcfc

aomarks force-pushed the amd-loader branch from e0455dc to b1d1cc8 Compare May 1, 2018 01:18

aomarks added 3 commits April 30, 2018 18:19

Add minification.

58b8913

More refactoring.

465fc17

Add WCT tests, and fix some bugs.

591c845

aomarks added 7 commits April 30, 2018 18:19

Update README.

972ca9a

Reset loader state for testing.

babed32

Add error handling and test.

b088075

Reorder some things and rename reset to _reset.

75fefc0

Rename amd-loader to esm-amd-loader (package dir).

aa32009

Rename amd-loader to esm-amd-loader (TS source).

750788c

Rename amd-loader to esm-amd-loader (everything else).

30d0a1f

aomarks force-pushed the amd-loader branch from b1d1cc8 to 30d0a1f Compare May 1, 2018 01:19

aomarks changed the title ~~[WIP] New AMD-style module loader.~~ New AMD-style module loader. May 1, 2018

Get esm-amd-loader wct tests running.

06f3fc4

Disable AMD loader tests on Windows because WCT is broken.

aomarks force-pushed the amd-loader branch from 0bb9ff7 to 06f3fc4 Compare May 1, 2018 18:59

aomarks merged commit e28a201 into master May 1, 2018

aomarks deleted the amd-loader branch May 2, 2018 08:06

snyk-bot mentioned this pull request Mar 17, 2020

[Snyk] Security upgrade typescript-json-schema from 0.25.1 to 0.38.0 gcloud-lerralice/tools#32

Open

Exnadella mentioned this pull request May 14, 2022

[Snyk] Security upgrade typescript-json-schema from 0.25.1 to 0.38.0 Exnadella/tools#35

Open


		# @polymer/amd-loader

		A JavaScript library that loads AMD-style modules.

		@@ -0,0 +1,237 @@
		type modCallback = (...args: {}[]) => void;

		@@ -0,0 +1,2 @@
		/node_modules/
		/lib/

New AMD-style module loader. #254

New AMD-style module loader. #254

Conversation

aomarks commented Apr 26, 2018 • edited Loading

bicknellr 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

justinfagnani commented Apr 26, 2018

aomarks commented Apr 26, 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

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

aomarks 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

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

aomarks commented Apr 28, 2018

aomarks commented Apr 26, 2018 •

edited

Loading