API Fetch: Support custom fetch handlers #12365

mmtr · 2018-11-27T15:24:54Z

Description

This PR adds support to the api-fetch package for using custom fetch handlers instead of using window.fetch, which is still the default fetch handler.

This is needed in applications like Calypso which uses wpcom.js for performing requests to the REST API.

However, right now the only way of doing that is by breaking the middleware chain, which involves to register again the middlewares that are already registered by Gutenberg. Furthermore, if the middleware is not exposed publicly (like userLocaleMiddleware) we need to copy the code of the middleware into Calypso. This is not ideal, since it has a maintenance cost every time there is an upgrade of the api-fetch package.

With this change, it will be possible to define a custom fetch handler with the setFetchHandler method that will override the last middleware executed, so the chain is not broken.

More context in Automattic/wp-calypso#28271.

How has this been tested?

With unit tests

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

youknowriad · 2018-11-27T15:30:48Z

Can't we just invert the order of some middlewares and ensure the registered middlewares are added last (just before the raw handler) which means breaking the middleware chain is ok?

Any reason this is not a valid alternative?

mmtr · 2018-11-27T15:45:58Z

Can't we just invert the order of some middlewares and ensure the registered middlewares are added last (just before the raw handler) which means breaking the middleware chain is ok?

Any reason this is not a valid alternative?

Couldn't this be a breaking change for folks using custom middlewares? They can get different results if we run the custom middlewares at the end just before the raw handler.

For example, the same example provided in the README.md:

import apiFetch from '@wordpress/api-fetch';

apiFetch.use( ( options, next ) => {
	const start = Date.now();
	const result = next( options );
	result.then( () => {
		console.log( 'The request took ' + Date.now() - start );
	} );
	return result;
} );

The output will display now a shorter amount of time, because the built-in middlewares have been already executed.

I know this is a not realistic example, but wanted to illustrate that we might cause unexpected results to people using custom middlewares.

I think it's better if we avoid a breaking change so that's why I went for the finalware approach suggested by @jsnajdr.

youknowriad · 2018-11-27T15:48:47Z

I'm on the fence personally, I see this as a small breaking change that won't probably affect anyone in the WP world. The alternative is a new API which duplicates a little bit the purpose of middlewares. Thoughts @aduth @gziolo?

aduth · 2018-11-27T16:40:28Z

packages/api-fetch/src/index.js

+			return nextMiddleware( nextOptions, next );
+		}
+
+		// Next middleware is the last one, so no next arg is passed


It's a subtle change in behavior that affects only internal usage; but I'm curious if there's any need or precedent in middleware patterns for omitting the next argument for the last in the stack vs. passing a noop (guaranteeing a middleware to receive next as a function).

I did this mainly to simplify the expected arguments in the toHaveBeenCalledWith call of the unit test checking the behavior when using a custom fetch handler.

But you're right, I didn't remember the noop function, which is also easy to test and guarantees consistency since all middlewares will receive a next function.

I'll apply this change in a bit.

Done in 9bce879

aduth · 2018-11-27T16:59:06Z

I'm pretty okay with what's proposed here, more-so I think than your suggestion @youknowriad . My unease is likely with any "dependency" between middlewares, in this case some middleware which is expected to take place at a point before the default fetch behavior, because of the fragility in relying on a specific order in which middlewares are applied, and an assumption of full control over which middlewares are applied (at least in a plugin landscape, this is very unlikely to be guaranteed).

Another option I could see is to leverage our existing pattern of middleware creators, i.e. createFetchMiddleware. The difference between this and other middleware creators like createNonceMiddleware is that we'd want some default to exist when not explicitly provided, and optionally maybe a maximum of one fetch middleware in the middlewares stack.

Pseudo-code:

function createFetchMiddleware( handler ) {
	const middleware = ( options ) => handler( options );
	middleware.fetchMiddleware = true;
	return middleware;
}

function apiFetch( options ) {
	const steps = [
		...middlewares,
		fetchAllMiddleware,
		httpV1Middleware,
		namespaceEndpointMiddleware,
		userLocaleMiddleware,
	];

	if ( ! some( steps, 'fetchMiddleware' ) ) {
		steps.push( createFetchMiddleware( defaultHandler ) );
	}

	// ...
}

This middleware's a bit unique in how we'd expect it to (a) always exist and (b) be the terminating middleware. Under those assumptions, we could modify the implementation a bit to either test only the last of the middleware stack, or change the behavior of runMiddleware to test for and guarantee one and only one fetchHandler call.

aduth · 2018-11-27T17:02:59Z

packages/api-fetch/package.json

 		"@wordpress/i18n": "file:../i18n",
-		"@wordpress/url": "file:../url"
+		"@wordpress/url": "file:../url",
+		"lodash": "^4.17.10"


I'm not strictly opposed to it since it's a library which in a wp-admin context will most certainly be loaded anyways, it's worth pointing out that for the npm distributable, this has a sizeable impact in bundle size, for what effectively amounts to a usage of what can otherwise be expressed as () => {}.

The "called with" test, if an argument for having something with referential equality, is not one I think we need to guarantee anyways. The guarantee is simply that the middleware is passed a function (expect.any( Function )).

👍 I think we can avoid avoiding pulling all of lodash as a requirement here, especially since we're only using noop. @mmtr generally we need to be a bit more careful on the trade-offs costs of pulling packages into libraries vs applications. (Think of the standalone case of someone only wanting to use the api-fetch package for instance).

In theory this becomes a non-issue if we revisit something like #9374

Anyway, I agree it is not worth it to include a new dependency only for using noop, so I removed it in 3f94782 and used expect.any( Function ) as @aduth suggested for checking that a function is passed to the last middleware.

gwwar · 2018-11-27T18:51:20Z

Looks like one failure in

Summary of all failing tests
FAIL test/e2e/specs/blocks/quote.test.js (23.51s)
  ● Quote › can be converted to headings
    expect(value).toMatchSnapshot()
    Received value does not match stored snapshot "Quote can be converted to headings 3".
    - Snapshot
    + Received
    @@ -4,8 +4,8 @@
      
      <!-- wp:heading -->
      <h2>two</h2>
      <!-- /wp:heading -->
      
    - <!-- wp:heading -->
    - <h2>cite</h2>
    - <!-- /wp:heading -->"
    + <!-- wp:quote -->
    + <blockquote class="wp-block-quote"><p></p><cite>cite</cite></blockquote>
    + <!-- /wp:quote -->"
      131 | 		await page.click( '[data-type="core/quote"]' );
      132 | 		await convertBlock( 'Heading' );
    > 133 | 		expect( await getEditedPostContent() ).toMatchSnapshot();

I've restarted the e2e run, but we should investigate if it fails again.

gwwar · 2018-11-27T19:04:34Z

Nice work @mmtr! This will help reduce a lot of future maintenance overhead on our side.

@aduth if I'm hearing this correctly folks are okay with the finalware approach here? Besides getting tests green and smoke testing for regressions, are there any other high level items to discuss/work through?

gziolo · 2018-11-28T09:00:37Z

Pseudo-code:
function createFetchMiddleware( handler ) {
	const middleware = ( options ) => handler( options );
	middleware.fetchMiddleware = true;
	return middleware;
}

function apiFetch( options ) {
	const steps = [
		...middlewares,
		fetchAllMiddleware,
		httpV1Middleware,
		namespaceEndpointMiddleware,
		userLocaleMiddleware,
	];

	if ( ! some( steps, 'fetchMiddleware' ) ) {
		steps.push( createFetchMiddleware( defaultHandler ) );
	}

	// ...
}
This middleware's a bit unique in how we'd expect it to (a) always exist and (b) be the terminating middleware. Under those assumptions, we could modify the implementation a bit to either test only the last of the middleware stack, or change the behavior of runMiddleware to test for and guarantee one and only one fetchHandler call.

I'm personally fine with the currently proposed solution as it is going to be easier to use for developers. By using the explicit override setFetchHandler, we can avoid having some hidden flags which need to be included to ensure it is going to override the default fetch middleware (What if such flag is not provided?). It also makes it clear that it replaces an existing middleware. Now, that raises a very important question, what do we do about all other default middlewares: fetchAll, httpV1, namespace, userLocale? I think we should answer this question first before we proceed as it my impact the public API shape. I would personally prefer to see something closer to apiFetch.replaceFetchMiddleware( myFetchMiddleware ) as the name of the method, but I could also evolve into apiFetch.replaceMiddleware( 'fetch', myFetchMiddleware ) if we want to make it more flexible. If we pick the 2nd approach we can limit it to fetch for this PR so we could iterate later.

jsnajdr · 2018-11-28T09:34:23Z

There are two distinct concepts involved here:

fetch handler that actually executes the request. Default handler does a raw window.fetch and I can replace it with my own, e.g., one that sends messages to a proxy iframe. There's always only one handler active.
middleware that can modify the request, drop the request, or fork it onto multiple ones (like fetchAllMiddleware does). There are multiple middlewares, some built-in, some user-defined. Order matters.

The fetch handler is by definition the last function that sees a request and the first one that sees the response. It's not in the middle of anything. Calling it a "middleware" makes the user API confusing: the middleware chain becomes an array of objects of different type and with different constraints, and it allows the user to write nonsense code:

// does order matter for `fetchHandler`? or is it detected and moved to the end?
[ middle1, fetchHandler, middle2 ]
// only one `fetchHandler` ever runs, but the API lets me specify multiple ones?
[ middle1, fetchHandler1, middle2, fetchHandler2, middle3 ]

That's why I'd strongly prefer a setFetchHandler function. Just my 2¢.

mmtr · 2018-11-28T10:44:25Z

The fetch handler is by definition the last function that sees a request and the first one that sees the response. It's not in the middle of anything. Calling it a "middleware" makes the user API confusing.

I agree with this.

We add the fetch handler to the list of middlewares, but it is intentionally added at the end, so it is not really a middleware to me.

I think is ok to have it in the middlewares chain if that simplifies the code (same function will execute both middlewares and fetch) as long as we do that only internally and we don't expose publicly it as a middleware, since that can confuse the developers.

gziolo · 2018-11-28T12:14:52Z

The fetch handler is by definition the last function that sees a request and the first one that sees the response. It's not in the middle of anything. Calling it a "middleware" makes the user API confusing.

The current implementation is the reason why I got confused as someone not familiar with the internals of the apiFetch implementation. If you want to make it explicit that is the last step in the chain and it isn't middle-ware, you should take it out of the logic that runs middlewares. I guess it would also allow to avoid using the following condition:

if ( steps.length > ( index + 1 ) ) {

which is targeted to the fetch handler.

mmtr · 2018-11-28T16:01:01Z

If you want to make it explicit that is the last step in the chain and it isn't middle-ware, you should take it out of the logic that runs middlewares

I think that the confusion comes from the name used in the runMiddleware function. This function is actually executing a step of the chain, not a middleware, and the chain is composed by the following steps:

User defined middlewares
Built-in middlewares
Fetch handler

Based on this, I applied some changes in 78cc4a3 which renames runMiddleware to runStep, and makes more obvious that the chain is composed by first the user defined middlewares, then the built-in middlewares and lastly the fetch handler.

aduth · 2018-11-28T16:48:53Z

packages/api-fetch/README.md

+apiFetch.setFetchHandler( ( options ) => {
+	const { url, path, data, method } = options;
+
+	return axios({


Nitpick:

Arrays and Function Calls
Always include extra spaces around elements and arguments:

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#arrays-and-function-calls

aduth · 2018-11-28T16:56:41Z

packages/api-fetch/src/index.js

-		...middlewares,
+	];
+
+	const steps = [


I'd normally not have brought it up, but since you did some refactoring here, I'm a bit curious why we construct as much as we do here on every call to apiFetch. Why not just initialize the scoped middlewares to include fetchAllMiddleware et al? I think by eliminating steps we could even make clearer that the fetch handler is the terminating behavior. Looking at runStep's creation by index + 1, it took me more than a few seconds to understand how we'd not risk some undefined value being called as a function.†

† In fact, while this probably won't normally occur, I think an error would be thrown if the fetch handler actually called next ?

diff --git a/packages/api-fetch/src/test/index.js b/packages/api-fetch/src/test/index.js index d993a8b63..0149a9efb 100644 --- a/packages/api-fetch/src/test/index.js +++ b/packages/api-fetch/src/test/index.js @@ -196,5 +196,6 @@ describe( 'apiFetch', () => { expect( customFetchHandler ).toHaveBeenCalledWith( { path: '/random?_locale=user', }, expect.any( Function ) ); + expect( customFetchHandler.mock.calls[ 0 ][ 1 ] ).not.toThrow(); } ); } );

● apiFetch › should not use the default fetch handler when using a custom fetch handler expect(function).not.toThrow() Expected the function not to throw an error. Instead, it threw: TypeError: nextStep is not a function 136 | const nextStep = steps[ index ]; 137 | const next = runStep( index + 1 ); > 138 | return nextStep( nextOptions, next ); | ^ 139 | }; 140 | 141 | return runStep( 0 )( options ); at nextStep (packages/api-fetch/src/index.js:138:10) at Object.<anonymous> (node_modules/expect/build/to_throw_matchers.js:51:9) at Object.throwingMatcher [as toThrow] (node_modules/expect/build/index.js:320:33) at Object.toThrow (packages/api-fetch/src/test/index.js:199:57) 197 | path: '/random?_locale=user', 198 | }, expect.any( Function ) ); > 199 | expect( customFetchHandler.mock.calls[ 0 ][ 1 ] ).not.toThrow(); | ^ 200 | } ); 201 | } ); 202 | at Object.toThrow (packages/api-fetch/src/test/index.js:199:57)

I'd normally not have brought it up, but since you did some refactoring here, I'm a bit curious why we construct as much as we do here on every call to apiFetch. Why not just initialize the scoped middlewares to include fetchAllMiddleware et al? I think by eliminating steps we could even make clearer that the fetch handler is the terminating behavior. Looking at runStep's creation by index + 1, it took me more than a few seconds to understand how we'd not risk some undefined value being called as a function.†

† In fact, while this probably won't normally occur, I think an error would be thrown if the fetch handler actually called next ?

After consulting with @mmtr, I pushed a few commits to sort out my concerns.

aduth · 2018-11-28T18:15:41Z

The fetch handler is by definition the last function that sees a request and the first one that sees the response. It's not in the middle of anything. Calling it a "middleware" makes the user API confusing.

I agree with this.

We add the fetch handler to the list of middlewares, but it is intentionally added at the end, so it is not really a middleware to me.

In which case, we probably shouldn't pass it next. Reflected in 41d712d.

youknowriad · 2018-11-29T08:02:17Z

packages/api-fetch/src/index.js

+
+let fetchHandler = defaultFetchHandler;
+
+function setFetchHandler( newFetchHandler ) {


I know this file was already lacking so JSDocs, but maybe a good time to start adding those.

Added in e2dc3d3

gziolo

Thanks for iterating on the implementation. It should be all clear now that fetchHandler differs from middlewares.

gziolo · 2018-11-29T09:29:43Z

packages/api-fetch/src/index.js


 function registerMiddleware( middleware ) {
-	middlewares.push( middleware );
+	middlewares.unshift( middleware );


Nice, much easier to follow than using reverse on the list of middlewares 👍

gziolo · 2018-11-29T09:32:10Z

packages/api-fetch/src/index.js

+}
+
+function apiFetch( options ) {
+	const steps = [ ...middlewares, fetchHandler ];


Thanks for updating this code here. It is now clear that fetchHandler is something unrelated to middlewares.

gziolo · 2018-11-29T09:44:26Z

packages/api-fetch/src/test/index.js

+
+		apiFetch.use( ( actualOptions, next ) => {
+			expect( actualOptions ).toBe( expectedOptions );
+			expect( actualOptions ).toEqual( expectedOptions );


From https://jestjs.io/docs/en/expect#tobevalue:

Use .toBe to compare primitive values or to check referential identity of object instances. It calls Object.is to compare values, which is even better for testing than === strict equality operator.

Nit: I think the 2nd check (. toEqual) is obsolete in this test.

You're right. toEqual is useful for checking the "deep" equality of two different objects. Removed in e2dc3d3

From https://jestjs.io/docs/en/expect#tobevalue:

Use .toBe to compare primitive values or to check referential identity of object instances. It calls Object.is to compare values, which is even better for testing than === strict equality operator.

Nit: I think the 2nd check (. toEqual) is obsolete in this test.

The idea was to check that neither the reference changed nor the value mutated. I think toBe could return true even if the value was directly mutated?

var a = {} var b = a b.foo = 1 Object.is( a, b ) // true

b.foo = 1 also implies a.foo = 1, so if the reference is the same, the value will be the same:

const a = {}; const b = a; b.foo = 1; expect( a ).toBe( b ); // true expect( a ).toEqual( b ); // true expect( a.foo ).toEqual( 1 ); // true

I'd thought there could be some case where one is not sufficient to capture both conditions, but on reflection I think toBe should be enough. 👍

youknowriad · 2018-12-03T10:07:37Z

The scope of 4.7 (WordPress 5.0.1) is frozen at this stage. We need approvals from release leads to include anything. This is targeted for 4.8 at the moment.

gwwar · 2019-01-01T01:02:10Z

Any idea of when this might land? I do wonder if a more complex model like gitflow would be interesting for folks. It's more overhead but we'd be able to continue development on a particular release while another one is being polished for specific bugfixes.

gziolo · 2019-01-02T10:15:27Z

Yes, we discussed using gitflow like approach. @youknowriad proposed a bit simplified flow 2 weeks ago which we want to try first:
https://make.wordpress.org/core/2018/12/19/gutenberg-commits-and-merge-status-update/

We want to start simple and iterate. The fact that we manage packages from Gutenberg and they need also to feed WordPress core is suboptimal. However, this is a bigger issue as we can't move codebase easily to WordPress core codebase because it is svn based, so our tooling won't work with it out of the box. The other way around is also a complex task to accomplish. If you have some ideas how to make it all work with Lerna and all requirements listed in the shared post, we can team up and improve the flow.

As of today, this PR is planned for 4.9 which should align with 5.1 release cycle of WordPress core. I think beta is planned for the middle of January, which is where we would publish packages to npm with this patch included.

gwwar · 2019-01-02T17:06:19Z

Thanks for the links and estimate @gziolo!

We want to start simple and iterate.

💯 What's interesting in this case is that there are several release cycles to consider.

Each npm package may have it's own independent release cycle and can publish at any time
Gutenberg too has a release cycle (faster than WordPress) which is a tag at some point in time, and hopefully reflects some subset of pinned npm packages (vs having code that has not been published)
Picking a Gutenberg version ties into what might align on a WordPress release update.

I can maybe add a comment on that post if I can clarify my thoughts a bit (and maybe draw a few diagrams). But really great to hear folks are thinking about this.

gziolo · 2019-01-03T13:16:31Z

Each npm package may have it's own independent release cycle and can publish at any time

That would be ideal, however, with how use Lerna to manage monorepo at the moment, it's hard to achieve when you publish from master as it bumps version for all modified packages. You get more control when you use a branch for releases but then you need to cherry-pick commits which has its own set of cons.

Gutenberg too has a release cycle (faster than WordPress) which is a tag at some point in time, and hopefully reflects some subset of pinned npm packages (vs having code that has not been published)
Picking a Gutenberg version ties into what might align on a WordPress release update.

Actually, Gutenberg 4.8 is going to contain more changes than what will go into WordPress 5.0.3. Plugin is going to be updated of the master but WordPress core will get only bug fixes and performance improvements selected from the recent commits since 5.0.2 release. It's confusing but makes a lot of sense in the current setup. Again, npm needs to reflect what lands in WordPress core so we try to avoid releasing new features until we reach WordPress 5.1.x cycle.

youknowriad · 2019-01-04T09:38:43Z

Thanks all for your work here.

mmtr added the [Package] API fetch /packages/api-fetch label Nov 27, 2018

mmtr self-assigned this Nov 27, 2018

mmtr requested review from gwwar, rodrigoi, vindl and youknowriad November 27, 2018 15:24

mmtr mentioned this pull request Nov 27, 2018

Gutenberg: add required api middlewares Automattic/wp-calypso#28271

Closed

aduth reviewed Nov 27, 2018

View reviewed changes

gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement. labels Nov 28, 2018

mmtr force-pushed the update/api-fetch-custom-fetch-handler branch from 3f94782 to 1fe31aa Compare November 28, 2018 10:47

aduth reviewed Nov 28, 2018

View reviewed changes

aduth approved these changes Nov 28, 2018

View reviewed changes

youknowriad reviewed Nov 29, 2018

View reviewed changes

youknowriad approved these changes Nov 29, 2018

View reviewed changes

gziolo approved these changes Nov 29, 2018

View reviewed changes

youknowriad modified the milestones: 4.7, 4.8 Dec 3, 2018

mmtr force-pushed the update/api-fetch-custom-fetch-handler branch from 0ee5821 to 86b5cfe Compare December 17, 2018 15:56

mmtr mentioned this pull request Dec 17, 2018

Gutenberg: verify that needed core api middleware is running Automattic/wp-calypso#28777

Closed

youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018

mmtr and others added 12 commits December 19, 2018 13:05

API Fetch: Support custom fetch handlers

d0c0c4e

Remove duplicated word

5d1a92f

Revert package-lock.json

7d0f115

Use noop as next middleware in the last middleware in the stack

8c10f7c

Remove lodash dependency

13c7088

Better naming for differentiating steps from middlewares

27c951d

Add extra espace around argument

3a285de

API Fetch: Add test to assure against throwing next function

4422de2

API Fetch: Add test to guarantee user-defined middleware order

ecdac11

API Fetch: Assign built-in middlewares on scope variable

9b9424b

API Fetch: Treat fetchHandler as terminator, not middleware

5bce8a6

Add JSDoc and remove not needed check

a5bbeec

mmtr force-pushed the update/api-fetch-custom-fetch-handler branch from 86b5cfe to a5bbeec Compare December 19, 2018 12:06

youknowriad merged commit 5d34eb9 into master Jan 4, 2019

mmtr mentioned this pull request Jan 4, 2019

Gutenberg: Avoid to break the apiFetch middleware chain Automattic/wp-calypso#29909

Closed

aduth deleted the update/api-fetch-custom-fetch-handler branch January 25, 2019 21:37

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

API Fetch: Support custom fetch handlers (#12365)

619ce5e

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

API Fetch: Support custom fetch handlers (#12365)

bc09193


		let fetchHandler = defaultFetchHandler;

		function setFetchHandler( newFetchHandler ) {

API Fetch: Support custom fetch handlers #12365

API Fetch: Support custom fetch handlers #12365

Uh oh!

Conversation

mmtr commented Nov 27, 2018

Description

How has this been tested?

Checklist:

Uh oh!

youknowriad commented Nov 27, 2018

Uh oh!

mmtr commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented Nov 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Nov 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwwar Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwwar commented Nov 27, 2018

Uh oh!

gwwar commented Nov 27, 2018

Uh oh!

gziolo commented Nov 28, 2018

Uh oh!

jsnajdr commented Nov 28, 2018

Uh oh!

mmtr commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo commented Nov 28, 2018

Uh oh!

mmtr commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmtr Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mmtr commented Nov 27, 2018 •

edited

Loading

gwwar Nov 27, 2018 •

edited

Loading

mmtr commented Nov 28, 2018 •

edited

Loading

mmtr commented Nov 28, 2018 •

edited

Loading

aduth Nov 28, 2018 •

edited

Loading

mmtr Nov 29, 2018 •

edited

Loading

gwwar commented Jan 1, 2019 •

edited

Loading

gwwar commented Jan 2, 2019 •

edited

Loading