Data Layer: Fix POST-ing `formData` with an `apiVersion` while introducing `apiNamespace` #13028

bperson · 2017-04-12T13:50:06Z

Ok so this started as part of my trial where I need to use the wp/v2 namespace: #12733.

I initially figured it was just a matter of adding apiNamespace in the http action creator and quickly ended up in the deep hole of understanding why we weren't passing query for POST: https://github.com/Automattic/wp-calypso/blob/master/client/state/data-layer/wpcom-http/actions.js#L38

I just saw @dmsnell approach in #13022 which probably is better as we're passing only one of apiNamespace / apiVersion, but I still feel like there is value in 976dec9 and f4e4115, 33c7293 is included as context for why I ended up here :) I am pretty sure #12839 breaks what #12135 tried to do.

blowery · 2017-04-12T14:27:57Z

client/state/data-layer/wpcom-http/index.js

+			: successes.forEach( handler => dispatch( extendAction( handler, successMeta( nextData ) ) ) );
+	};
+
+	const request = fetcherMap( method, wpcom.req )( action, requestCallback );


blowery · 2017-04-12T14:28:01Z

client/state/data-layer/wpcom-http/index.js

+		query,
+		callback
+	),
+	POST: ( { body = {}, formData, path, query = {}, }, callback ) => wpcomReq.post(


love it. makes all this much more explicit.

blowery · 2017-04-12T14:33:14Z

client/state/data-layer/wpcom-http/test/index.js

+
+		it( 'should send with query', () => {
+			getFooAction.query = {
+				apiVersion: 'v2.0',


i think apiVersion doesn't have a leading v. should be 2.0 i think?

blowery · 2017-04-12T14:33:55Z

client/state/data-layer/wpcom-http/test/index.js

+
+		it( 'should send formData with query', () => {
+			postFooAction.formData = [ [ 'foo', 'bar' ], ];
+			postFooAction.query = { apiVersion: 'v1.1' };


ditto, i think it's just 1.1, not v1.1.

see https://github.com/Automattic/wpcom.js/blob/master/index.js#L81

and https://github.com/Automattic/wpcom-xhr-request/blob/master/index.js#L139

Yep, I think I got confused with the default value used for apiVersion in the http action creator: https://github.com/Automattic/wp-calypso/blob/master/client/state/data-layer/wpcom-http/actions.js#L23

Now I wonder if there is a special case for v1 or if we should also remove the v in there? In #13022 @dmsnell got rid of the default but we're still using v1 in the tests, might be worth tackling it there as well.

dmsnell · 2017-04-12T16:14:40Z

client/state/data-layer/wpcom-http/actions.js

+	query: Object.assign(
+		{ apiVersion },
+		query,
+		apiNamespace && { apiNamespace },


note that this will break requests which provide an apiNamespace as they will fail if both values are present 😄

dmsnell · 2017-04-12T16:18:26Z

client/state/data-layer/wpcom-http/test/index.js

+
+describe( '#fetcherMap', () => {
+	const wpcomReq = {};
+	const noopCallback = () => {};


seems awkward to name this noopCallback instead of just noop.

note also that lodash.noop also exists

TIL, will use lodash.noop :)

dmsnell · 2017-04-12T16:25:02Z

client/state/data-layer/wpcom-http/test/index.js

+			getFooAction.query = {
+				apiVersion: '2.0',
+				apiNamespace: 'wp/v2',
+			};


this kind of mutation irks me because it hides the things that aren't here. interested at all in rewriting immutably?

const action = { ...baseAction, query: { param: 'test' } }; // or since it's not too big we could envision just writing it out each time const wpcom = { get: spy(), post: spy(), }; beforeEach( () => wpcom.get.reset() ); const action = { method: 'GET', path: '/foo', query: { apiVersion: '1.3' } }

we actually wrote fewer lines this way overall…

I honestly don't feel strongly about either :) I went that way (let + new spy-s in beforeEach to match what was already there for dispatch / next). If we go with const + resets, it's probably worth updating the entire file.

sounds fair either way. you can blame me for any inconsistencies 😄

dmsnell

Thanks for this work @bperson - I kind of like the general changes you made. Please note that this is unmergable due to the current bug in the way it assigns apiVersion and apiNamespace but I think you were ware of that.

Something about the fetcher map stands out to me. It's not something I can quite pin down, but I am going to stew on it a bit. Please nag me if I delay too much (will be AFK for the next few days)

any reason to chose wpcomReq instead of just req?

bperson · 2017-04-12T16:45:42Z

Please note that this is unmergable due to the current bug in the way it assigns apiVersion and apiNamespace but I think you were ware of that.

Yep for the unmerge-ability, I will revert 33c7293 and it's probably worth waiting for #13022 to land and rebasing on top of it.

Something about the fetcher map stands out to me.

It's definitely becoming a weird beast: a kind of meta-mapper of args :D . On top of that the wpcomReq thrown in there for testing doesn't help.

any reason to chose wpcomReq instead of just req?

I feel like req will be mis-interpreted as a request, which is what we label the actual result of a wpcom.req.get|post call. Considering it's there only for testing, I feel like being super-explicit about what we expect there is a safer bet.

dmsnell · 2017-04-12T16:46:54Z

I feel like req will be mis-interpreted as a request, which is what we label the actual result of a wpcom.req.get|post call. Considering it's there only for testing, I feel like being super-explicit about what we expect there is a safer bet.

just curious. .req is what wpcom.js uses internally

nylen · 2017-04-14T10:47:46Z

just curious. .req is what wpcom.js uses internally

I find this pretty confusing as well.

dmsnell · 2017-04-18T05:49:12Z

@bperson please note that #13022 is ready for merging and I'll deploy it as soon as I get back to more usual network conditions. I think this should remove some of the size of this PR even, but it will require some rework/rebasing.

bperson · 2017-04-21T08:26:04Z

@dmsnell np, I'm watching your PR and will rebase / push force this branch (warning if someone's actively using this branch :) ). I don't expect that much change with the current status (since I've reverted my commit for the addition of apiNamespace).

bperson · 2017-04-21T08:28:19Z

I find this pretty confusing as well.

@nylen do you find confusing the usage of req for something that isn't a request in wpcom? or using wpcomReq in this PR? If it's the latter, np I'll update this after the rebase to use req :)

dmsnell · 2017-04-25T00:08:03Z

@bperson thanks again for your help with this! I just wanted to let you know that #13022 has merged and this should be ready to rework/rebase against those updates 😄

matticbot · 2017-04-25T11:22:15Z

@bperson This PR needs a rebase

…n the `query` wpcom's `req.post` has the signature: `params, [query], body, [callback]` which means positionnal arguments will move around based on the function's arity: 4: `params, query, body, callback` 3: `params, query, body` or `params, body, callback` 2: `params, body` With that in mind, the use of `compact` in the data-layer considerably complexifies the situation, as we can hit those various arities for many combinations of arguments. This tries to simplify the process while fixing a few edge cases already encountered in a few past PRs (#12135, #12839): If you want to pass `formData` (in `params`) with an `apiVersion` (in `query`), you NEED to pass a `body`, but any truth-y body will overwrite the `formData` in the HTTP request. This disqualifies the usage of `compact` as you - sometimes - want to pass a false-y values. (PR #12135) If you want to just pass an `apiVersion` (in `query`), you need - at least - another argument to have an arity of 3, which means you need to pass an empty body (PR #12839)

nylen · 2017-04-27T18:52:05Z

@nylen do you find confusing the usage of req for something that isn't a request in wpcom?

Yes, but it's probably too late to change that now.

or using wpcomReq in this PR? If it's the latter, np I'll update this after the rebase to use req :)

I think wpcomReq is fine, though if I were doing all of this again I might choose wpcomHttp or something like that. But it might actually be worse to make that change here only, because we have this existing convention already.

When I see a variable named req I expect it to be an instance of a completed or in-progress HTTP request, with properties like headers and body.

dmsnell · 2017-04-27T19:06:30Z

When I see a variable named req I expect it to be an instance of a completed or in-progress HTTP request, with properties like headers and body.

@nylen just a side note, but that's not usually the case, not with wacom.js at least, as the .req object contains the .get(), .post(), and the misleading .delete() and .put() methods and aren't themselves actual network request objects

nylen · 2017-04-27T19:15:58Z

@nylen just a side note, but that's not usually the case, not with wacom.js at least, as the .req object contains the .get(), .post(), and the misleading .delete() and .put() methods and aren't themselves actual network request objects

Right, this is exactly what I am taking issue with. It would be better if we had called that http. But again that ship has sailed, it'd be a ton of work to change it at this point without much benefit.

bperson · 2017-04-27T19:21:21Z

@nylen just a side note, but that's not usually the case, not with wacom.js at least, as the .req object contains the .get(), .post(), and the misleading .delete() and .put() methods and aren't themselves actual network request objects

I think that's what we're finding confusing :) Outside of wpcom.js, req is usually a request object containing stuff related to a request, not a "request factory".

As another sidenote, I thought about bypassing the entire req debate ... by not using it and replacing its usage with sendRequest directly. The get, post, put and del functions are really just wrapper around JS' past inability to have default arguments [0] (and overall decent arguments handling ;) ), but using sendRequest directly is deprecated [1], so I don't think it's worth spamming our consoles :D

[0] https://github.com/Automattic/wpcom.js/blob/master/lib/util/request.js
[1] https://github.com/Automattic/wpcom.js/blob/master/index.js#L173

dmsnell · 2017-06-21T15:01:41Z

@bperson do we have a status update on this? is there work to be done? are you waiting on us?

bperson · 2017-06-21T18:34:37Z

I still think there is a need to make it more explicit how the various arities of the wpcom interface are handled. I've just re-read #14651 which seems to have become the hub for this issue?.

For now though, this isn't a blocker to close my trial and it's probably safe to say that someone with more experience in calypso could lead that effort way better than me (trial-er for life 🤘 ) so maybe this should be closed and the ideas re-used in a stronger effort in July as mentioned on #14651?

matticbot added OSS Citizen [Size] L Large sized issue labels Apr 12, 2017

bperson changed the title ~~Data Layer: Fix POST-ing formData/~~ Data Layer: Fix POST-ing formData with an apiVersion while introducing apiNamespace Apr 12, 2017

bperson requested review from dmsnell, blowery and donnapep April 12, 2017 13:51

designsimply added API [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 12, 2017

blowery reviewed Apr 12, 2017

View reviewed changes

dmsnell reviewed Apr 12, 2017

View reviewed changes

dmsnell requested changes Apr 12, 2017

View reviewed changes

matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Apr 13, 2017

dmsnell mentioned this pull request Apr 18, 2017

Data Layer: Allow setting apiNamespace in requests #13022

Merged

matticbot added the [Status] Needs Rebase label Apr 25, 2017

bperson force-pushed the update/data-layer-api-namespace branch from 75bcd31 to 4bb79f6 Compare April 27, 2017 15:07

matticbot added [Size] L Large sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Apr 27, 2017

bperson mentioned this pull request May 25, 2017

Data Layer: Fix for video poster not updating due to incorrect form data #14102

Closed

dmsnell mentioned this pull request Jun 1, 2017

Data Layer HTTP: Fix request query for WP REST API POST #14651

Merged

bperson closed this Jun 22, 2017

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 22, 2017

dmsnell deleted the update/data-layer-api-namespace branch June 22, 2017 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data Layer: Fix POST-ing `formData` with an `apiVersion` while introducing `apiNamespace` #13028

Data Layer: Fix POST-ing `formData` with an `apiVersion` while introducing `apiNamespace` #13028

bperson commented Apr 12, 2017

blowery Apr 12, 2017

blowery Apr 12, 2017 •

edited

blowery Apr 12, 2017

blowery Apr 12, 2017

blowery Apr 12, 2017

blowery Apr 12, 2017

bperson Apr 12, 2017

dmsnell Apr 12, 2017

dmsnell Apr 12, 2017

bperson Apr 12, 2017

dmsnell Apr 12, 2017

bperson Apr 12, 2017

dmsnell Apr 12, 2017

dmsnell left a comment

bperson commented Apr 12, 2017 •

edited

dmsnell commented Apr 12, 2017

nylen commented Apr 14, 2017

dmsnell commented Apr 18, 2017

bperson commented Apr 21, 2017

bperson commented Apr 21, 2017

dmsnell commented Apr 25, 2017

matticbot commented Apr 25, 2017

nylen commented Apr 27, 2017

dmsnell commented Apr 27, 2017

nylen commented Apr 27, 2017

bperson commented Apr 27, 2017 •

edited

dmsnell commented Jun 21, 2017

bperson commented Jun 21, 2017

Data Layer: Fix POST-ing formData with an apiVersion while introducing apiNamespace #13028

Data Layer: Fix POST-ing formData with an apiVersion while introducing apiNamespace #13028

Conversation

bperson commented Apr 12, 2017

Choose a reason for hiding this comment

blowery Apr 12, 2017 • edited

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

dmsnell left a comment

Choose a reason for hiding this comment

bperson commented Apr 12, 2017 • edited

dmsnell commented Apr 12, 2017

nylen commented Apr 14, 2017

dmsnell commented Apr 18, 2017

bperson commented Apr 21, 2017

bperson commented Apr 21, 2017

dmsnell commented Apr 25, 2017

matticbot commented Apr 25, 2017

nylen commented Apr 27, 2017

dmsnell commented Apr 27, 2017

nylen commented Apr 27, 2017

bperson commented Apr 27, 2017 • edited

dmsnell commented Jun 21, 2017

bperson commented Jun 21, 2017

Data Layer: Fix POST-ing `formData` with an `apiVersion` while introducing `apiNamespace` #13028

Data Layer: Fix POST-ing `formData` with an `apiVersion` while introducing `apiNamespace` #13028

blowery Apr 12, 2017 •

edited

bperson commented Apr 12, 2017 •

edited

bperson commented Apr 27, 2017 •

edited