Handle /timeouts W3C + MJSONWP proxying #231
Conversation
lib/jsonwp-proxy/proxy.js
Outdated
| * at a time. So if we're using W3C and proxying to MJSONWP and there's more than one timeout type | ||
| * provided in the request, we need to do 3 proxies and combine the result | ||
| * | ||
| * @param {Array} timeoutObjects List of MJSONWP + W3C compatible timeouts request objects |
There was a problem hiding this comment.
also no return description
| }; | ||
| if (protocol === W3C) { | ||
| for (let [type, ms] of _.toPairs(baseObj)) { | ||
| if (_.isNumber(ms)) { |
There was a problem hiding this comment.
should we also check if type is defined?
There was a problem hiding this comment.
type will always be defined because it's the key of an object
lib/jsonwp-proxy/proxy.js
Outdated
|
|
||
| /** | ||
| * Proxy an array of timeout objects and merge the result | ||
| * @param {Array} timeoutObjects List of W3C+MJSONWP compatible /timeouts requests |
There was a problem hiding this comment.
same documentation issue here
| let protocol = this.getProtocolFromResBody(resBody); | ||
|
|
||
| // If we got a non-MJSONWP response, return the result, nothing left to do | ||
| if (protocol !== MJSONWP) { |
There was a problem hiding this comment.
I think this is redundant, since there will be anyway only one item in the array in case of w3c
There was a problem hiding this comment.
If there's an array of timeout objects like this:
[
{pageLoad: 1, script: 2, implicit: 3, type: 'pageLoad', ms: 1},
{pageLoad: 1, script: 2, implicit: 3, type: 'script', ms: 2},
{pageLoad: 1, script: 2, implicit: 3, type: 'implicit', ms: 3},
]...then we need to stop after the first call if it's W3C because it's done.
| return MJSONWP; | ||
| } else if (util.hasValue(resBody.value)) { | ||
| return W3C; | ||
| } |
There was a problem hiding this comment.
what if both conditions fail?
There was a problem hiding this comment.
I'd like to return it as undefined so that it's just an unknown protocol.
lib/jsonwp-proxy/proxy.js
Outdated
| async command (url, method, body = null) { | ||
| let response; | ||
| let resBody; | ||
| try { | ||
| [response, resBody] = await this.proxy(url, method, body); | ||
| if (url === '/timeouts' && method.toLowerCase() === 'post') { |
There was a problem hiding this comment.
I don't really like the fact that the url is hardcoded here. We might make it more flexible
lib/jsonwp-proxy/proxy.js
Outdated
| }); | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
return [{
...baseObj,
type,
ms,
}];
There was a problem hiding this comment.
else will be redundant in such case
|
Fixes pushed |
| async command (url, method, body = null) { | ||
| let response; | ||
| let resBody; | ||
| try { | ||
| [response, resBody] = await this.proxy(url, method, body); | ||
| if (url.endsWith('timeouts') && method.toLowerCase() === 'post') { |
There was a problem hiding this comment.
maybe endsWith('/timeouts')? what if a user is trying to get the 'timeouts' attribute of an element? (i know that's a 'get', just saying...)
There was a problem hiding this comment.
@jlipps Yeah I could change it to that.
The /timeouts endpoint is a special case because the syntax differs between W3C and MJSONWP
W3C takes a body like this:
MJSONWP takes a body like this
Since W3C can send > 1 timeout type, we need to have special logic so that if a W3C /timeouts request is being proxied to a MJSONWP /timeouts endpoint, we need to proxy 1 request for each timeout
The way this PR handles that is by creating an array of timeout request objects like this:
If the incoming request is W3C, like this:
It will generate an array that looks like this:
If the incoming request is MJSONWP, like this:
It will generate an array that looks like this:
object and proxy that one too
not continue to the next
(related to: appium/appium#11016)