ngUpgrade AoT #12239

petebacondarwin · 2016-10-12T13:44:01Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

petebacondarwin · 2016-10-12T14:08:01Z

karma-js.conf.js

@@ -23,7 +23,7 @@ module.exports = function(config) {

      'node_modules/core-js/client/core.js',
      // include Angular v1 for upgrade module testing
-      'node_modules/angular/angular.min.js',
+      'node_modules/angular/angular.js',


TODO: remember to revert this change

can revert it?

@mhevery said it can stay un-minified

mhevery

Awesome start and great progress!

mhevery · 2016-10-12T18:13:25Z

modules/@angular/upgrade/index.ts

@@ -12,5 +12,6 @@
 * Entry point for all public APIs of the upgrade package.
 */
 export * from './src/upgrade';
+export * from './src/upgrade_module';


Don't export all. Instead list which items specifically you want to export. Otherwise $injectorFactory and friends will be part of public API.

Right, OK. I'll fix up.

mhevery · 2016-10-12T18:15:50Z

modules/@angular/upgrade/src/upgrade_module.ts

+@NgModule({
+  providers: [
+    { provide: '$injector', useFactory: $injectorFactory },
+    { provide: '$rootScope', useFactory: $rootScopeFactory, deps: ['$injector']},


@chuckjaz Notice how we have to explicitly list $rootScopeFactory and $compileFactory in future we will have many more. It would be nice to just have one function which does it all, but we get an Error:

Metadata collected contains an error that will be reported at runtime: Function calls are not supported. Consider replacing the function or lambda with a reference to an exported function

I thought we supported this, but apparently not. Could you work with @petebacondarwin and help him resolve it?

@mhevery Sure.

@petebacondarwin Can I see the function that you are trying to use? It might be easy to transform.

mhevery · 2016-10-12T18:16:17Z

modules/@angular/upgrade/src/upgrade_module.ts

+  providers: [
+    { provide: '$injector', useFactory: $injectorFactory },
+    { provide: '$rootScope', useFactory: $rootScopeFactory, deps: ['$injector']},
+    { provide: '$compile', useFactory: $compileFactory, deps: ['$injector']}


Should we re-export all of the $* items?

There are lots of them...
Perhaps we could just provide a recipe in the docs for people to add their own to their subclass of UpgradeModule?

mhevery · 2016-10-12T18:17:15Z

modules/@angular/upgrade/src/upgrade_module.ts

+/**
+ * UpgradeModule is the base class for the module that will contain all the
+ * information about what Angular providers and component are to be bridged
+ * between Angular 1 and Angular 2+


We will have to update this doc with an example. We now require that full API documentation will be included at the time of submission of the PR.

//cc @IgorMinar

These docs were mostly placeholders to help with my understanding.
We can add more as the work stabilizes

mhevery · 2016-10-12T18:19:33Z

modules/@angular/upgrade/src/upgrade_module.ts

+   * Usage:
+   *
+   * ```
+   * angular1Module.factory('someService', ng2ProviderFactory(SomeService))


turn these into running examples. See: #11773 for how the docs should look / work.

will do as things stabilize

mhevery · 2016-10-12T18:34:18Z

modules/@angular/upgrade/src/upgrade_module.ts

+    function directiveFactory(
+        ng1Injector: angular.IInjectorService,
+        parse: angular.IParseService): angular.IDirective {
+      const NG2_INJECTOR = 'ng2.Injector';


make sure we have consistent naming. either ng2.foo or ng2Foo

ng2.Injector is taken directly from the rest of the ngUpgrade codebase. I don't know how much of a breaking change it would be that code if we changed it and I think that it would be nice to keep the two forms of the library consistent.

But as @robwormald points out we probably shouldn't have ng2 in this...

Any better naming ideas for this?

I see the point with ng2. So maybe we should just do $injector and ng.injector? as the two ways. Renaming it would be a breaking change, even thought it is not part of the public API so it is unlikely that anyone would be using it. But sounds like the ngUpgradeAoT should use ng.Injector

mhevery · 2016-10-12T18:34:47Z

modules/@angular/upgrade/src/upgrade_module.ts

+    this.ngZone = ng2Injector.get(NgZone);
+  }
+
+  ngDoBootstrap() {}


add comment as to which this method is here.

strangely I did have a comment here but it seems to have failed to make it into the commit that was pushed.

mhevery · 2016-10-12T18:35:27Z

modules/@angular/upgrade/src/upgrade_module.ts

+
+    // Only add the config if it is there
+    // QUESTION? Do we really need this param?
+    if (config) {


why is this necessary? Wouldn't angular.bootstrap take care of it?

mhevery · 2016-10-12T18:37:08Z

modules/@angular/upgrade/test/upgrade_aot_spec.ts

+
+    it('should have angular 1 loaded', () => expect(angular.version.major).toBe(1));
+
+    it('should instantiate ng2 in ng1 template and project content', async(() => {


@juliemr we are having a hard time making the async work here. We are getting timeout. Could you work with @petebacondarwin on figuring out why it is failing to stabilize?

I think this may have resolved itself...

mhevery · 2016-10-12T18:39:31Z

modules/@angular/upgrade/test/upgrade_aot_spec.ts

+          html('<div>{{ \'ng1[\' }}<ng2>~{{ \'ng-content\' }}~</ng2>{{ \']\' }}</div>');
+
+      platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => {
+        ref.instance.bootstrapNg1(element, [ng1Module.name]);


Why do it here, rather than in the Ng2Module constructor?

Moving the call to bootstrapNg1 outside the constructor makes the custom UpgradeModule more reusable. It doesn't tie it into always bootstrapping on a specific element.

I see, good point.

petebacondarwin · 2016-10-12T22:03:44Z

modules/@angular/upgrade/src/aot/ng1_service_provider.ts

+ */
+export function ng1ServiceProvider({provide, ng1Token}: {provide: any, ng1Token: any}): FactoryProvider {
+  return {provide: provide, useFactory: (i: angular.IInjectorService) => i.get(ng1Token), deps: ['$injector']};
+}


shouldn't this helper fall foul of the same problems with not being allowed to use a non-exported named function as a factory?
cc @mhevery & @chuckjaz

Yes. The lambda here doesn't help you.

petebacondarwin · 2016-10-13T11:27:22Z

modules/@angular/upgrade/test/upgrade_aot_spec.ts

-      //          }
-      //        };
-      //      });
+      it('should properly run cleanup when ng1 directive is destroyed', async(() => {


@gkalpak can you take a look and see if this test makes sense. The previous commented out test that this replaces is very different and in I think doesn't even work properly.

The test makes sense. I think the previous test tried to do the same.
One thing I noticed is that the previous test calls ref.dispose() at the end. Is it necessary? What was the purpose?

The ref.dispose() came from how the adaptor worked. I don't believe it is necessary here. We are already calling platform.dispose() in an afterEach block.

googlebot · 2016-10-13T20:50:22Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

petebacondarwin · 2016-10-13T20:52:51Z

@gkalpak please do the honors for the googlebot

gkalpak · 2016-10-13T21:01:41Z

modules/@angular/upgrade/src/angular_js.ts

@@ -51,7 +51,7 @@ export interface IRootScopeService {
  [key: string]: any;
 }
 export interface IScope extends IRootScopeService {};
-export interface IAngularBootstrapConfig {}
+export type IAngularBootstrapConfig = IInjectable;


But it is not the same. IAngularBootstrapConfig is the configuration that can be passed to angular.bootstrap and can contain stringDi: true/false for example. (It can also contain debugInfoEnabled, but that is undocumented and used internally.) Essentially, it is just an object with specific properties.

So, it should probably be changed to:

export interface IAngularBootstrapConfig { strictDi?: boolean; }

petebacondarwin · 2016-10-14T06:09:35Z

Oops

On 13 Oct 2016 22:04, "Georgios Kalpakas" notifications@github.com wrote:

@gkalpak commented on this pull request.

In modules/@angular/upgrade/src/angular_js.ts
#12239:

@@ -51,7 +51,7 @@ export interface IRootScopeService {
[key: string]: any;
}
export interface IScope extends IRootScopeService {};
-export interface IAngularBootstrapConfig {}
+export type IAngularBootstrapConfig = IInjectable;

So, it should probably be changed to:

export interface IAngularBootstrapConfig {
strictDi?: boolean;
}

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12239, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA9J-80qsnq9YruZZabnZy49Ezy2RyVks5qzpzTgaJpZM4KUxSw
.

gkalpak · 2016-10-14T07:28:12Z

@googlebot, I confirm that I'm okay with my commits being contributed to Google 😃

gkalpak · 2016-10-14T12:00:48Z

modules/@angular/upgrade/test/upgrade_aot_spec.ts

@@ -342,6 +345,9 @@ export function main() {
        platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => {
          const adapter = ref.injector.get(Ng1Adapter) as Ng1Adapter;
          adapter.bootstrapNg1(element, [ng1Module.name]);
+          // the fact that the body contains the correct text means that the
+          // downgraded component was able to access the moduleInjector
+          // (since there is no other injector in this system)


Still not convinced 😛

Trouble is until we have upgradeComponent working we can't do better than this test

Unless you can show otherwise

mhevery

Excellent progress sir!

mhevery · 2016-10-14T13:54:14Z

modules/@angular/upgrade/src/angular_js.ts

@@ -6,20 +6,30 @@
 * found in the LICENSE file at https://angular.io/license
 */

+export type Ng1Token = string;


what is the value of aliasing string to Ng1Token

I was looking forward to if/when we might allow non-string tokens in ng1

mhevery · 2016-10-14T13:55:14Z

modules/@angular/upgrade/src/angular_js.ts

@@ -6,20 +6,30 @@
 * found in the LICENSE file at https://angular.io/license
 */

+export type Ng1Token = string;
+
+export interface IAnnotatedFunction extends Function {


as soon as #11773 gets in, you will no longer need local definitions like this one.

Well I didn't know about https://www.npmjs.com/package/@types/angular.
@gkalpak and I probably wasted a bit of time over the last few days tweaking this file.

I think we would need to do some work with that repository as I am not entirely happy with all of the hand crafted types there.

mhevery · 2016-10-14T13:56:04Z

modules/@angular/upgrade/src/aot/downgrade_component.ts

+
+export function downgradeComponent(info: ComponentInfo) : Function {
+
+  const idPrefix = `NG2_UPGRADE_${downgradeCount++}_`;


drop NG2 as we said no 2 in the names.

mhevery · 2016-10-14T13:56:36Z

modules/@angular/upgrade/src/aot/downgrade_component.ts

+
+    return {
+      restrict: 'E',
+      require: '?^' + UPGRADE_MODULE_INJECTOR,


change UPGRADE_MODULE_INJECTOR to COMPONENT_INJECTOR

OK, do we still leave the UPGRADE_MODULE_INJECTOR constant for when we are accessing the top level injector? E.g. $injector.get(UPGRADE_MODULE_INJECTOR)

mhevery · 2016-10-14T14:01:17Z

modules/@angular/upgrade/src/aot/ng1_module.ts

+ */
+@NgModule({
+  providers: [
+    Ng1Adapter,


Why is this broken up to a separate class? Why no just part of the module? I am on the fence, and wanted to understand your reasoning.

I gave my reasoning on the ngUpgrade Slack channel and in the commit for this 6beed8d

Basically, I personally found using the NgModule class to do the bootstrap confusing.

In my understanding NgModules are about statically defining information about the compilation and are not "active" in the sense that you call them to do stuff.

I could be convinced to rejoin them but I think with the right name for the service it makes it adheres better to the Principle of Least Surprise.

mhevery · 2016-10-14T14:04:13Z

modules/@angular/upgrade/src/constants.ts

@@ -7,10 +7,11 @@
 */

 export const NG2_COMPILER = 'ng2.Compiler';
-export const NG2_INJECTOR = 'ng2.Injector';
+export const UPGRADE_MODULE_INJECTOR = 'ng2.Injector';
 export const NG2_COMPONENT_FACTORY_REF_MAP = 'ng2.ComponentFactoryRefMap';


Except that it is still used by the JIT UpgradeAdapter

mhevery · 2016-10-14T14:04:26Z

modules/@angular/upgrade/src/constants.ts

@@ -7,10 +7,11 @@
 */

 export const NG2_COMPILER = 'ng2.Compiler';
-export const NG2_INJECTOR = 'ng2.Injector';
+export const UPGRADE_MODULE_INJECTOR = 'ng2.Injector';
 export const NG2_COMPONENT_FACTORY_REF_MAP = 'ng2.ComponentFactoryRefMap';
 export const NG2_ZONE = 'ng2.NgZone';


mhevery · 2016-10-14T14:06:57Z

modules/@angular/upgrade/test/upgrade_aot_spec.ts

+          entryComponents: [Ng2Component]
+        })
+        class Ng2Module  {
+          ngDoBootstrap() {}


nice thing about extending was that one did not have to write ngDoBootstrap method.

That is true. But it still rankled that we were the UpgradeModule in its free standing class form as an NgModule import; and then also creating a subclass of UpgradeModule purely to inherit this one method.

We are stuck between two variants of unwanted boilerplate.
But it did occur to me that we could get Angular-CLI to generate this, so either way it shouldn't matter too much.

mhevery · 2016-10-14T14:08:54Z

modules/@angular/upgrade/test/upgrade_aot_spec.ts

+        // the ng1 app module that will consume the downgraded component
+        const ng1Module = angular.module('ng1', [])
+          // create an ng1 facade of the ng2 component
+          .directive('ng2', downgradeComponent({ selector: 'ng2', type: Ng2Component }));


Why do we have to pass in the selector? it should not be needed.

Also can we rename type to component?

You are right. I was being lazy because I was reusing DowngradeNg2ComponentAdapter from the JIT version which accepts a ComponentInfo type, which requires the selector and type properties.

In fact the only use of info.selector in that class is when reporting an error.

We should refactor this and remove the selector.

mhevery · 2016-10-14T14:09:36Z

modules/@angular/upgrade/test/upgrade_aot_spec.ts

+            html('<div>{{ \'ng1[\' }}<ng2>~{{ \'ng-content\' }}~</ng2>{{ \']\' }}</div>');
+
+        platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => {
+          const adapter = ref.injector.get(Ng1Adapter) as Ng1Adapter;


By having Ng1Adapter as a separate class, there is more things to import. Still on the fence about that.

Let's mull it over during the weekend. Happy to revert if it still feels wrong.

mhevery · 2016-10-14T16:00:48Z

modules/@angular/upgrade/test/upgrade_aot_spec.ts

+
+        @NgModule({
+          declarations: [Ng1Component, Ng2Component],
+          imports: [BrowserModule]


Ng1Module => UpgradeModule

alexeagle · 2016-10-18T00:27:40Z

modules/@angular/upgrade/src/metadata.ts

@@ -27,8 +27,8 @@ export interface AttrProp {
 export interface ComponentInfo {


not part of your change - but the import of DirectiveResolver above causes @angular/upgrade to depend on @angular/compiler so my AoT example is still getting the compiler at runtime, which makes the app big. Maybe a Closure Compiler issue but it's easier to trim the dependency.
@mhevery this suggests the aot version of upgrade should be a separate package so we can avoid the reflection dependencies?

upgrade -> platform-browser-dynamic -> compiler happens too

alexeagle · 2016-10-19T20:44:51Z

karma-js.conf.js

@@ -23,7 +23,7 @@ module.exports = function(config) {

      'node_modules/core-js/client/core.js',
      // include Angular v1 for upgrade module testing
-      'node_modules/angular/angular.min.js',
+      'node_modules/angular/angular.js',


can revert it?

alexeagle · 2016-10-19T20:57:48Z

tools/public_api_guard/upgrade/index.d.ts

+
+/** @experimental */
+export declare class UpgradeModule {
+    $injector: angular.IInjectorService;


this only works if you have this local angular1 typings to make it match.
Real users with angular 1 and angular 2 typings will find a structural mismatch here. Needs to be any

We agreed to wait for it to break stuff

@mhevery

This commit introduces a new API to the ngUpgrade module, which is compatible with AoT compilation. Primarily, it removes the dependency on reflection over the Angular 2 metadata by introducing an API where this information is explicitly defined, in the source code, in a way that is not lost through AoT compilation. This commit is a collaboration between @mhevery (who provided the original design of the API); @gkalpak & @petebacondarwin (who implemented the API and migrated the specs from the original ngUpgrade tests) and alexeagle (who provided input and review). This commit is an starting point, there is still work to be done: * add more documentation * validate the API via internal projects * align the ngUpgrade compilation of A1 directives closer to the real A1 compiler * add more unit tests * consider support for async `templateUrl` A1 upgraded components Closes angular#12239

googlebot · 2016-10-19T21:38:47Z

CLAs look good, thanks!

@mhevery

This commit introduces a new API to the ngUpgrade module, which is compatible with AoT compilation. Primarily, it removes the dependency on reflection over the Angular 2 metadata by introducing an API where this information is explicitly defined, in the source code, in a way that is not lost through AoT compilation. This commit is a collaboration between @mhevery (who provided the original design of the API); @gkalpak & @petebacondarwin (who implemented the API and migrated the specs from the original ngUpgrade tests) and @alexeagle (who provided input and review). This commit is an starting point, there is still work to be done: * add more documentation * validate the API via internal projects * align the ngUpgrade compilation of A1 directives closer to the real A1 compiler * add more unit tests * consider support for async `templateUrl` A1 upgraded components Closes angular#12239

@mhevery

This commit introduces a new API to the ngUpgrade module, which is compatible with AoT compilation. Primarily, it removes the dependency on reflection over the Angular 2 metadata by introducing an API where this information is explicitly defined, in the source code, in a way that is not lost through AoT compilation. This commit is a collaboration between @mhevery (who provided the original design of the API); @gkalpak & @petebacondarwin (who implemented the API and migrated the specs from the original ngUpgrade tests) and @alexeagle (who provided input and review). This commit is an starting point, there is still work to be done: * add more documentation * validate the API via internal projects * align the ngUpgrade compilation of A1 directives closer to the real A1 compiler * add more unit tests * consider support for async `templateUrl` A1 upgraded components Closes angular#12239

petebacondarwin · 2016-10-19T21:41:27Z

karma-js.conf.js

@@ -23,7 +23,7 @@ module.exports = function(config) {

      'node_modules/core-js/client/core.js',
      // include Angular v1 for upgrade module testing
-      'node_modules/angular/angular.min.js',
+      'node_modules/angular/angular.js',


@mhevery said it can stay un-minified

petebacondarwin · 2016-10-19T21:43:21Z

modules/@angular/upgrade/src/constants.ts

@@ -7,10 +7,11 @@
 */

 export const NG2_COMPILER = 'ng2.Compiler';
-export const NG2_INJECTOR = 'ng2.Injector';
+export const UPGRADE_MODULE_INJECTOR = 'ng2.Injector';


I think we are not using these constants in the new AoT code.

petebacondarwin · 2016-10-19T21:49:24Z

tools/public_api_guard/upgrade/index.d.ts

+
+/** @experimental */
+export declare class UpgradeModule {
+    $injector: angular.IInjectorService;


We agreed to wait for it to break stuff

petebacondarwin · 2016-10-19T21:49:34Z

modules/@angular/upgrade/test/aot/test_helpers.ts

+    upgrade.bootstrap(element, [ng1Module.name]);
+    return upgrade;
+  });
+}


@mhevery - we should consider adding this helper to the public API

@mhevery

This commit introduces a new API to the ngUpgrade module, which is compatible with AoT compilation. Primarily, it removes the dependency on reflection over the Angular 2 metadata by introducing an API where this information is explicitly defined, in the source code, in a way that is not lost through AoT compilation. This commit is a collaboration between @mhevery (who provided the original design of the API); @gkalpak & @petebacondarwin (who implemented the API and migrated the specs from the original ngUpgrade tests) and @alexeagle (who provided input and review). This commit is an starting point, there is still work to be done: * add more documentation * validate the API via internal projects * align the ngUpgrade compilation of A1 directives closer to the real A1 compiler * add more unit tests * consider support for async `templateUrl` A1 upgraded components Closes angular#12239

angular-automatic-lock-bot · 2019-09-12T02:21:35Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

petebacondarwin added the comp: upgrade label Oct 12, 2016

petebacondarwin assigned petebacondarwin, alexeagle, mhevery and gkalpak Oct 12, 2016

googlebot added the cla: yes label Oct 12, 2016

petebacondarwin commented Oct 12, 2016

View reviewed changes

mhevery suggested changes Oct 12, 2016

View reviewed changes

mhevery changed the title ~~Ngupgrade aot~~ ngUpgrade AoT Oct 12, 2016

petebacondarwin commented Oct 12, 2016

View reviewed changes

petebacondarwin commented Oct 13, 2016

View reviewed changes

googlebot added cla: no and removed cla: yes labels Oct 13, 2016

gkalpak reviewed Oct 13, 2016

View reviewed changes

ericmartinezr mentioned this pull request Oct 14, 2016

AoT with Upgrade Adapter? #12292

Closed

gkalpak reviewed Oct 14, 2016

View reviewed changes

mhevery suggested changes Oct 14, 2016

View reviewed changes

mhevery reviewed Oct 14, 2016

View reviewed changes

alexeagle reviewed Oct 18, 2016

View reviewed changes

petebacondarwin force-pushed the ngupgrade-aot branch from 1e939a8 to 579246a Compare October 18, 2016 09:26

alexeagle added cla: yes and removed cla: no labels Oct 19, 2016

alexeagle approved these changes Oct 19, 2016

View reviewed changes

chore(tslint.json): semicolon rule expects an array

a2d3564

petebacondarwin force-pushed the ngupgrade-aot branch from 3e91299 to e7d76b5 Compare October 19, 2016 21:38

petebacondarwin force-pushed the ngupgrade-aot branch from e7d76b5 to 01dfa08 Compare October 19, 2016 21:40

petebacondarwin force-pushed the ngupgrade-aot branch from 01dfa08 to 1230810 Compare October 19, 2016 21:46

petebacondarwin commented Oct 19, 2016

View reviewed changes

alexeagle added pr_state: LGTM action: merge The PR is ready for merge by the caretaker labels Oct 19, 2016

alxhub closed this in d6791ff Oct 19, 2016

petebacondarwin deleted the ngupgrade-aot branch July 14, 2017 11:18

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019


		it('should have angular 1 loaded', () => expect(angular.version.major).toBe(1));

		it('should instantiate ng2 in ng1 template and project content', async(() => {


		export function downgradeComponent(info: ComponentInfo) : Function {

		const idPrefix = `NG2_UPGRADE_${downgradeCount++}_`;

		@@ -27,8 +27,8 @@ export interface AttrProp {
		export interface ComponentInfo {

ngUpgrade AoT #12239

ngUpgrade AoT #12239

Conversation

petebacondarwin commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhevery 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

gkalpak Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Oct 13, 2016

petebacondarwin commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Oct 14, 2016

@gkalpak commented on this pull request.

gkalpak commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhevery 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

petebacondarwin Oct 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin Oct 14, 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

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

googlebot commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Oct 13, 2016 •

edited

Loading

petebacondarwin Oct 14, 2016 •

edited

Loading

petebacondarwin Oct 14, 2016 •

edited

Loading