Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data Layer: Fix POSTing data in data layer #12135

Merged
merged 2 commits into from
Mar 16, 2017
Merged

Conversation

donnapep
Copy link
Contributor

Passing empty objects for body and query causes form data to not be POSTed correctly, since wpcom runs explicit checks against undefined, or missing, parameters. Not passing these parameters to wpcom at all resolves the problem.

@donnapep donnapep self-assigned this Mar 14, 2017
@matticbot
Copy link
Contributor

@dmsnell
Copy link
Contributor

dmsnell commented Mar 14, 2017

Looks like a reasonable fix @donnapep - I'm less familiar with the POST requests and wpcom.js in general. Let's test this but I think it's all looking great

@donnapep donnapep added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 15, 2017
@donnapep donnapep requested a review from dmsnell March 15, 2017 10:35
@donnapep
Copy link
Contributor Author

@dmsnell Looks like only Plans is using the HTTP layer. I've tested that and it's still working. Reader has some code to use it for tags, but it doesn't actually look like it's being called from anywhere yet. So I think we're good!

@donnapep donnapep changed the title Fix POSTing data in data layer Data Layer: Fix POSTing data in data layer Mar 15, 2017
@dmsnell
Copy link
Contributor

dmsnell commented Mar 15, 2017

@donnapep is the size -> total thing a part of superagent? MDN shows that the XMLHttpRequest onprogress event should have a property called size. also, if we have some browser incompatibility issues here we'll need to handle those properly

https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequestEventTarget/onprogress

@donnapep
Copy link
Contributor Author

@dmsnell Looks like the info at that link is incorrect. The spec indicates total and makes no mention of size. The example in this MDN doc also uses total.

I tested uploading on Chrome, FF, Safari and IE and it all works well.

Cheers!

@donnapep
Copy link
Contributor Author

Updated the MDN documentation. That was easy!

Copy link
Contributor

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

excellent work @donnapep, especially on updating the MDN docs!

also, I tested this on the live branch and it did not appear to break anything that I could see

@donnapep donnapep merged commit 856f143 into master Mar 16, 2017
@donnapep donnapep deleted the fix/data-layer-post branch March 16, 2017 18:41
@donnapep donnapep removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 16, 2017
@matticbot matticbot added the [Size] S Small sized issue label Mar 23, 2017
bperson added a commit that referenced this pull request Apr 27, 2017
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Size] S Small sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants