Support for Configurable Request Library

jbgrunewald commented 4 years ago

I've been having quite a few issues related to the got Library, some still unresolved, for versions of the client >= 3. Have we considered making the request library configurable? If this is something people are interested in, I'd be happy to take a crack at the PR.

The requirement for upgrading node versions to use the latest client is prohibitive in some cases, yet there are new API features we would like to take advantage of. It would be ideal if we kept got as the default transport, but allowed for a configuring our own. This can already be done to some degree right now, by reassigning the request function on the client object. However, there are some internal functions which are used in the request function and not exposed via the client, which would be very helpful to continue using. A specific one that comes to mind is the function for extracting the information from the Link header.

lpinca commented 4 years ago

I will update to got@11 in the next few days. Hopefully this new version will fix existing issues with got@10.

jbgrunewald commented 4 years ago

Thanks,

I experimented locally with an upgrade and it didn’t resolve the timeouts for me.

It’s still prohibitive for us to upgrade node versions right now. So far as I can tell the requirement for node >10 is due to the Got dependency.

James Grunewald

On May 9, 2020, at 9:54 AM, Luigi Pinca notifications@github.com wrote:

I will update to got@11 in the next few days. Hopefully this new version will fix existing issues with got@10.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

lpinca commented 4 years ago

Node.js < 10 is no longer supported (Node.js 8 reached EOL on December 31, 2019) so it doesn't make sense to support any version below 10.

jbgrunewald commented 4 years ago

I understand that node 10 is the latest with LTS status. That doesn’t change the business decision for us to spend the time to upgrade right now.

I understand the new versions of Got are relying on features only present in later node versions. Other than Got, what has a dependency on features from Node 10?

By making the request function configurable we can avoid this dependency. I can submit a pull request to demonstrate what I’m thinking. If we don’t want to merge, then we can use our own fork for now.

This also makes it possible to customize other things like adding retries or caching. For the caching it could be as simple as adding a wrapper around the existing request function.

James Grunewald

On May 9, 2020, at 10:05 AM, Luigi Pinca notifications@github.com wrote:

Node.js < 10 is no longer supported so it doesn't make sense to support any version below 10.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

lpinca commented 4 years ago

By making the request function configurable we can avoid this dependency.

This is not an option as errors depends on the library used and I don't like that.

jbgrunewald commented 4 years ago

I agree? The errors would ideally be http client independent. Since they aren’t, if someone wanted to override the request library, they would need to honor the current interface.

James Grunewald

On May 9, 2020, at 10:59 AM, Luigi Pinca notifications@github.com wrote:

By making the request function configurable we can avoid this dependency.

This is not an option as errors depends on library used and I don't like that.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

pcross616 commented 4 years ago

I too am getting lots of got Timeout errors, the only way I was able to not have it timeout is set the API concurrency to 1 and rateLimitCalls to 1.

I upgraded got to version 11 and no difference. It seem to do with a threading issue.

lpinca commented 4 years ago

got had a lot of issues recently which are being resolved in both got and Node.js core. Please be patient until all issues are solved.

If they persist I'm open to switching to node-fetch with a major version update.

lpinca commented 4 years ago

Refs: https://github.com/sindresorhus/got/issues/1232

szmarczak commented 4 years ago

Hey, Got maintainer here :wave: I've released recently got@11.1.3. I don't know of any major issue other than https://github.com/sindresorhus/got/issues/1220 (which got fixed by disabling the DNS cache by default, release 11.1.2) ~and https://github.com/sindresorhus/got/issues/1221 (unconfirmed bug)~.

Please try using a keep-alive agent

{
    agent: {
        https: new https.Agent({keepAlive: true})
    }
}

and let me know if that fixes the timeout errors.

jbgrunewald commented 4 years ago

My local testing with got 11.1.3 is returning json parsing errors.

at Object.exports.parseBody (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/as-promise/core.js:29:15)

szmarczak commented 4 years ago

All tests are passing with got@11.1.3...

jbgrunewald commented 4 years ago

The response I'm getting back seems to be for an HTML page, which may be a symptom of this client misusing the GOT API. Perhaps there's a change to the GOT client that wasn't updated in this client. I'm not a regular user of GOT, so it's a bit more difficult for me to diagnose the root cause right now.

What I do know is that versions 2.25.1 of this client which was using got 8.3.2 was and continues working without issue. On the 3.0.0 version of this client, it's using GOT 10.7.0. I'm getting request timeouts in this version of the client. Once I upgrade to GOT 11.3.1 there are parsing errors, and the response looks like an HTML page.

In all cases I'm using node 12.16.1 on MacOS Catalina 10.15.4. I want to emphasize, I'm assuming that the usage of GOT within this client is correct. If so, there's no reason to expect an HTML response to that request.

jbgrunewald commented 4 years ago

The response appears to be the homepage for the shop in this case. Which implies the path is not being formatted correctly in the requests. I'll keep digging.

jbgrunewald commented 4 years ago

I'm note sure what the issue is. I've looked over the documentation and do not see where this client is misusing the GOT API.

For the Async GOT request: URI: Is an object like => { "pathname": "/admin/api/2019-10/locations.json", "hostname": "james-jumping-jeans.myshopify.com", "protocol": "https:" }

The options are like => {"headers":{"User-Agent":"shopify-api-node/3.3.1","X-Shopify-Access-Token":"redacted"},"timeout":60000,"responseType":"json","retry":0,"method":"GET"}

The response to this should be a json response from the Shopify API, but I get the HTML for the shop page. This makes me think the URL object isn't properly formatting the request URI from the object passed in. The documentation says it honors the http.Request option object, and this input seems to satisfy that requirement. I also tried building a WHATWG URL, and that request failed as well.

WHATWG URL:

URL {
  href: 'https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json',
  origin: 'https://james-jumping-jeans.myshopify.com',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'james-jumping-jeans.myshopify.com',
  hostname: 'james-jumping-jeans.myshopify.com',
  port: '',
  pathname: '/admin/api/2019-10/locations.json',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

Error when attempting to use WHATWG URL:

connect ECONNREFUSED 127.0.0.1:443 RequestError: connect ECONNREFUSED 127.0.0.1:443
    at ClientRequest.<anonymous> (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/core/index.js:792:25)
    at Object.onceWrapper (events.js:418:26)
    at ClientRequest.emit (events.js:323:22)
    at ClientRequest.EventEmitter.emit (domain.js:482:12)
    at ClientRequest.origin.emit (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/@szmarczak/http-timer/dist/source/index.js:39:20)
    at TLSSocket.socketErrorListener (_http_client.js:426:9)
    at TLSSocket.emit (events.js:311:20)
    at TLSSocket.EventEmitter.emit (domain.js:482:12)
    at emitErrorNT (internal/streams/destroy.js:92:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:60:3)
    at processTicksAndRejections (internal/process/task_queues.js:84:21)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1137:16)
error: Unhandled exception in getResourceListHandler messageHandler error: [shopify][api] Error requesting api product.list: connect ECONNREFUSED 127.0.0.1:443 RequestError: connect ECONNREFUSED 127.0.0.1:443
    at ClientRequest.<anonymous> (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/core/index.js:792:25)
    at Object.onceWrapper (events.js:418:26)
    at ClientRequest.emit (events.js:323:22)
    at ClientRequest.EventEmitter.emit (domain.js:482:12)
    at ClientRequest.origin.emit (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/@szmarczak/http-timer/dist/source/index.js:39:20)
    at TLSSocket.socketErrorListener (_http_client.js:426:9)
    at TLSSocket.emit (events.js:311:20)
    at TLSSocket.EventEmitter.emit (domain.js:482:12)
    at emitErrorNT (internal/streams/destroy.js:92:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:60:3)
    at processTicksAndRejections (internal/process/task_queues.js:84:21)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1137:16) Error: [shopify][api] Error requesting api product.list: connect ECONNREFUSED 127.0.0.1:443 RequestError: connect ECONNREFUSED 127.0.0.1:443
    at ClientRequest.<anonymous> (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/core/index.js:792:25)
    at Object.onceWrapper (events.js:418:26)
    at ClientRequest.emit (events.js:323:22)
    at ClientRequest.EventEmitter.emit (domain.js:482:12)
    at ClientRequest.origin.emit (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/@szmarczak/http-timer/dist/source/index.js:39:20)
    at TLSSocket.socketErrorListener (_http_client.js:426:9)
    at TLSSocket.emit (events.js:311:20)
    at TLSSocket.EventEmitter.emit (domain.js:482:12)
    at emitErrorNT (internal/streams/destroy.js:92:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:60:3)
    at processTicksAndRejections (internal/process/task_queues.js:84:21)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1137:16)
    at api (/Users/jbgrunewald/work/services/ecom-plugin/src/plugins/shopify/api.js:67:11)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

szmarczak commented 4 years ago

@jbgrunewald Can you post an example so I can reproduce?

jbgrunewald commented 4 years ago

@szmarczak The example above shows the exact inputs being used with the exception of redacting the accessToken value for my test store. When I use the object form, I get an html response back. When I use the WHATWG URL it appears to be calling my loop back address in the error message. In both cases I'm expecting this to make a network call to the Shopify admin API and a JSON formatted response.

Can you confirm that the inputs otherwise look correct? If you want to test this end to end with this client, you'll need to setup a test store on Shopify and get an access token, or you can email me at jbgrunewald@gmail.com. I can share credentials for a test store that we can use to debug the issue.

Unfortunately, this is related to my day job. So for the time being I'm going to overload the request function with another library to add the functionality we need. @lpinca If you're inclined I can make a pull request with those changes, and an example of how I overloaded the request function.

lpinca commented 4 years ago

@jbgrunewald no thanks. Can you show some requests that fail? I've just tried on my test shop with Node.js 14.2.0 on macOS and it works as expected:

const Shopify = require('shopify-api-node');

const shopify = new Shopify({
  shopName: 'quuz',
  apiVersion: '2020-01',
  apiKey: 'b68c3a4bf44cd240511e5f4114f0be4d',
  password: '2d170167fc62551920ca60fe40a5e548'
});

(async function() {
  const locations = await shopify.location.list();

  console.log(locations);
})().catch(console.error);

[
  {
    id: 42043015,
    name: 'Via Roma 1',
    address1: 'Via Roma 1',
    address2: null,
    city: 'Rome',
    zip: '12345',
    province: 'Roma',
    country: 'IT',
    phone: '',
    created_at: '2017-04-26T03:47:08-04:00',
    updated_at: '2017-09-01T13:07:08-04:00',
    country_code: 'IT',
    country_name: 'Italy',
    province_code: 'RM',
    legacy: false,
    active: true,
    admin_graphql_api_id: 'gid://shopify/Location/42043015'
  }
]

jbgrunewald commented 4 years ago

@lpinca When I test the client in isolation it works for me as well... So it works in some scenarios.

When I run this inside my service I'm getting timeouts still for the latest version of the client. Apparently there's something specific to my environment that's not working. Since others are experiencing the timeouts, I don't think this is unique to my setup.

This is running node 12 and 14 with the 3.3.1 version of the shopify client.

error: Timeout awaiting 'request' for 60000ms GotError: Timeout awaiting 'request' for 60000ms
    at onError (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/request-as-event-emitter.js:137:29)
    at ClientRequest.<anonymous> (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/request-as-event-emitter.js:157:17)
    at ClientRequest.emit (events.js:323:22)
    at ClientRequest.EventEmitter.emit (domain.js:482:12)
    at ClientRequest.origin.emit (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/@szmarczak/http-timer/dist/source/index.js:39:20)
    at Timeout.timeoutHandler [as _onTimeout] (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/utils/timed-out.js:40:17)
    at listOnTimeout (internal/timers.js:551:17)
    at Timeout.timeoutHandler [as _onTimeout] (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/utils/timed-out.js:40:31)
    at listOnTimeout (internal/timers.js:551:17)
    at processTimers (internal/timers.js:492:7)
error: Unhandled exception in getResourceListHandler messageHandler error: [shopify][api][james-jumping-jeans.myshopify.com] Error requesting api product.list: Timeout awaiting 'request' for 60000ms GotError: Timeout awaiting 'request' for 60000ms
    at onError (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/request-as-event-emitter.js:137:29)
    at ClientRequest.<anonymous> (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/request-as-event-emitter.js:157:17)
    at ClientRequest.emit (events.js:323:22)
    at ClientRequest.EventEmitter.emit (domain.js:482:12)
    at ClientRequest.origin.emit (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/@szmarczak/http-timer/dist/source/index.js:39:20)
    at Timeout.timeoutHandler [as _onTimeout] (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/utils/timed-out.js:40:17)
    at listOnTimeout (internal/timers.js:551:17)
    at Timeout.timeoutHandler [as _onTimeout] (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/utils/timed-out.js:40:31)
    at listOnTimeout (internal/timers.js:551:17)
    at processTimers (internal/timers.js:492:7) Error: [shopify][api][james-jumping-jeans.myshopify.com] Error requesting api product.list: Timeout awaiting 'request' for 60000ms GotError: Timeout awaiting 'request' for 60000ms
    at onError (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/request-as-event-emitter.js:137:29)
    at ClientRequest.<anonymous> (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/request-as-event-emitter.js:157:17)
    at ClientRequest.emit (events.js:323:22)
    at ClientRequest.EventEmitter.emit (domain.js:482:12)
    at ClientRequest.origin.emit (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/@szmarczak/http-timer/dist/source/index.js:39:20)
    at Timeout.timeoutHandler [as _onTimeout] (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/utils/timed-out.js:40:17)
    at listOnTimeout (internal/timers.js:551:17)
    at Timeout.timeoutHandler [as _onTimeout] (/Users/jbgrunewald/work/services/ecom-plugin/node_modules/got/dist/source/utils/timed-out.js:40:31)
    at listOnTimeout (internal/timers.js:551:17)
    at processTimers (internal/timers.js:492:7)
    at api (/Users/jbgrunewald/work/services/ecom-plugin/src/plugins/shopify/api.js:67:11)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

Example of how the client is created:

const shopify = new Shopify({
  shopName: 'james-jumping-jeans',
  accessToken: 'readacted',
  presentmentPrices: true,
  autoLimit: true,
  apiVersion: '2019-10',
});

lpinca commented 4 years ago

Can you create a reproducible failing test case against the test shop above?

lpinca commented 4 years ago

According to https://github.com/MONEI/Shopify-api-node/issues/371#issuecomment-626298426 the issue might be related to running multiple requests in parallel so it might make sense to only test https://github.com/lpinca/stopcock and got together.

lpinca commented 4 years ago

This related issue https://github.com/MONEI/Shopify-api-node/issues/370#issue-605452748 says that timeouts also happens when using got directly.

It's hard to debug something when you can't reproduce the problem.

jbgrunewald commented 4 years ago

I can't seem to reproduce this outside of our server setup. We don't do any sort of processing on outbound requests though. I can see that this is happening with a single request, when I call for the locations. This is the first request made in this code path. I was able to make the exact same request using the request library and received a response without issue.

The error is very clear, the requests are timing out in the GOT library. If I find a way to reproduce in a simple script I'll share it.

szmarczak commented 4 years ago

You can access all the options used to make the request via error.response.request.options (Got 11, for Got 10 it's just error.options). Also, what are the timings? error.response.timings

szmarczak commented 4 years ago

You can also try enabling HTTP2 support. You need to pass http2: true in Got options. But you need Got 11 for that.

szmarczak commented 4 years ago

Can you try Node.js 14 with the decompress Got option set to false (there is a bug that is currently being fixed due to the stream.pipeline behavior change).

jbgrunewald commented 4 years ago

@szmarczak I printed the options at failure, and everything looked as expected.

{"headers":{"user-agent":"shopify-api-node/3.3.1","x-shopify-access-token":"redacted"},"prefixUrl":"","hooks":{"beforeError":[],"init":[],"beforeRequest":[],"beforeRedirect":[],"beforeRetry":[],"afterResponse":[]},"timeout":{"request":60000},"retry":{"limit":0,"methods":["GET","PUT","HEAD","DELETE","OPTIONS","TRACE"],"statusCodes":[408,413,429,500,502,503,504,521,522,524],"errorCodes":["ETIMEDOUT","ECONNRESET","EADDRINUSE","ECONNREFUSED","EPIPE","ENOTFOUND","ENETUNREACH","EAI_AGAIN"],"maxRetryAfter":null},"method":"GET","maxRedirects":10,"decompress":true,"isStream":false,"throwHttpErrors":true,"ignoreInvalidCookies":false,"cache":false,"responseType":"json","resolveBodyOnly":false,"followRedirect":true,"dnsCache":false,"useElectronNet":false,"methodRewriting":true,"allowGetBody":false,"_pagination":{"countLimit":null},"pathname":"/admin/api/2019-10/locations.json","hostname":"james-jumping-jeans.myshopify.com","protocol":"https:","url":"https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json"}

szmarczak commented 4 years ago

Ok. Can you try running Node.js 14 with the http2 Got option set to true and/or the decompress Got option set to false?

If you can provide the timings that would be great.

jbgrunewald commented 4 years ago

These are the results using node 14.2.0 and Got 10.7.0 with the http2: true option.

{"headers":{"user-agent":"shopify-api-node/3.3.1","x-shopify-access-token":"redacted"},"prefixUrl":"","hooks":{"beforeError":[],"init":[],"beforeRequest":[],"beforeRedirect":[],"beforeRetry":[],"afterResponse":[]},"timeout":{"request":60000},"retry":{"limit":0,"methods":["GET","PUT","HEAD","DELETE","OPTIONS","TRACE"],"statusCodes":[408,413,429,500,502,503,504,521,522,524],"errorCodes":["ETIMEDOUT","ECONNRESET","EADDRINUSE","ECONNREFUSED","EPIPE","ENOTFOUND","ENETUNREACH","EAI_AGAIN"],"maxRetryAfter":null},"method":"GET","maxRedirects":10,"decompress":true,"isStream":false,"throwHttpErrors":true,"ignoreInvalidCookies":false,"cache":false,"responseType":"json","resolveBodyOnly":false,"followRedirect":true,"dnsCache":false,"useElectronNet":false,"methodRewriting":true,"allowGetBody":false,"_pagination":{"countLimit":null},"pathname":"/admin/api/2019-10/locations.json","hostname":"james-jumping-jeans.myshopify.com","protocol":"https:","http2":true,"url":"https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json"}
{"start":1589328314581,"socket":1589328314582,"lookup":1589328314582,"connect":1589328314598,"secureConnect":1589328314616,"upload":1589328314616,"response":1589328314801,"error":1589328374585,"abort":1589328374585,"phases":{"wait":1,"dns":0,"tcp":16,"tls":18,"request":0,"firstByte":185,"total":60004}}

jbgrunewald commented 4 years ago

These are the results using node 14.2.0 and Got 10.7.0 with the decompress: false option.

{"headers":{"user-agent":"shopify-api-node/3.3.1","x-shopify-access-token":"redacted"},"prefixUrl":"","hooks":{"beforeError":[],"init":[],"beforeRequest":[],"beforeRedirect":[],"beforeRetry":[],"afterResponse":[]},"timeout":{"request":60000},"retry":{"limit":0,"methods":["GET","PUT","HEAD","DELETE","OPTIONS","TRACE"],"statusCodes":[408,413,429,500,502,503,504,521,522,524],"errorCodes":["ETIMEDOUT","ECONNRESET","EADDRINUSE","ECONNREFUSED","EPIPE","ENOTFOUND","ENETUNREACH","EAI_AGAIN"],"maxRetryAfter":null},"method":"GET","maxRedirects":10,"decompress":false,"isStream":false,"throwHttpErrors":true,"ignoreInvalidCookies":false,"cache":false,"responseType":"json","resolveBodyOnly":false,"followRedirect":true,"dnsCache":false,"useElectronNet":false,"methodRewriting":true,"allowGetBody":false,"_pagination":{"countLimit":null},"pathname":"/admin/api/2019-10/locations.json","hostname":"james-jumping-jeans.myshopify.com","protocol":"https:","url":"https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json"}
{"start":1589328553505,"socket":1589328553507,"lookup":1589328553508,"connect":1589328553523,"secureConnect":1589328553544,"upload":1589328553545,"response":1589328553995,"error":1589328613509,"abort":1589328613511,"phases":{"wait":2,"dns":1,"tcp":15,"tls":21,"request":1,"firstByte":450,"total":60006}}

szmarczak commented 4 years ago

These are the results using node 14.2.0 and Got 10.7.0 with the http2: true option.

There's no http2 support in Got 10. Please use Got 11.1.3.

These are the results using node 14.2.0 and Got 10.7.0 with the decompress: false option.

So it means that the response event is received, but there's no end (or Got doesn't catch it somehow). Can you try again with Got 11.1.3? Please try these:

    timeout: {
        // Global timeout.
        request: 60000,
        // If there's 1s inactivity of the socket, throw.
        socket: 1000
    },
    decompress: false

jbgrunewald commented 4 years ago

@szmarczak With 11.1.3, I haven't been receiving a timeout issue. I've been getting the incorrect response back for the input. I'm getting an html page (the page for the store), not the json formatted data I expect.

Here are the options and the timings:

{"method":"GET","retry":{"limit":0,"methods":["GET","PUT","HEAD","DELETE","OPTIONS","TRACE"],"statusCodes":[408,413,429,500,502,503,504,521,522,524],"errorCodes":["ETIMEDOUT","ECONNRESET","EADDRINUSE","ECONNREFUSED","EPIPE","ENOTFOUND","ENETUNREACH","EAI_AGAIN"],"maxRetryAfter":60000},"headers":{"user-agent":"shopify-api-node/3.3.1","x-shopify-access-token":"redacted","accept":"application/json"},"hooks":{"init":[],"beforeRequest":[],"beforeRedirect":[],"beforeRetry":[],"beforeError":[],"afterResponse":[]},"decompress":false,"throwHttpErrors":true,"followRedirect":true,"isStream":false,"responseType":"json","resolveBodyOnly":false,"maxRedirects":10,"prefixUrl":"","methodRewriting":true,"ignoreInvalidCookies":false,"http2":false,"allowGetBody":false,"rejectUnauthorized":true,"pagination":{"countLimit":null,"requestLimit":10000,"stackAllItems":true},"pathname":"/admin/api/2019-10/locations.json","hostname":"james-jumping-jeans.myshopify.com","protocol":"https:","username":"","password":"","url":"https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json","timeout":{"request":60000,"socket":1000}}
{"start":1589389670373,"socket":1589389670374,"lookup":1589389670375,"connect":1589389670396,"secureConnect":1589389670417,"upload":1589389670417,"response":1589389670570,"end":1589389670601,"phases":{"wait":1,"dns":1,"tcp":21,"tls":21,"request":0,"firstByte":153,"download":31,"total":228}}

The header and the url look correct. If I set those headers and the that url using any other request tool, I get the json formatted response from the API. I tested the behavior in the browser without the headers, and received an authorization error. The only way to receive the html response I'm receiving, from my manual testing, is if the hostname is used without the path.

jbgrunewald commented 4 years ago

@szmarczak I tried with 11.3.1 with the http2: true. The request was successful.

I'm assuming this isn't expected, and we won't need to add that flag to every request?

szmarczak commented 4 years ago

I tried with 11.3.1 with the http2: true. The request was successful.

That's good news.

I'm assuming this isn't expected, and we won't need to add that flag to every request?

It will come enabled by default in Got 12. But the request shouldn't fail even with having it disabled.

The only way to receive the html response I'm receiving, from my manual testing, is if the hostname is used without the path.

So it may be a URL parsing issue in Got...

With 11.1.3, I haven't been receiving a timeout issue. I've been getting the incorrect response back for the input. I'm getting an html page

Can you try Got 11.1.3 again with the same timeout option but having http2 removed and decompress still set tofalse? What's the response.req._header (raw header data including method, path etc.)?

jbgrunewald commented 4 years ago

@szmarczak It does look like there's a parsing issue without the http2 option.

Results with node 14.2.0 Got 11.1.3

{"method":"GET","retry":{"limit":0,"methods":["GET","PUT","HEAD","DELETE","OPTIONS","TRACE"],"statusCodes":[408,413,429,500,502,503,504,521,522,524],"errorCodes":["ETIMEDOUT","ECONNRESET","EADDRINUSE","ECONNREFUSED","EPIPE","ENOTFOUND","ENETUNREACH","EAI_AGAIN"],"maxRetryAfter":60000},"headers":{"user-agent":"shopify-api-node/3.3.1","x-shopify-access-token":"redacted","accept":"application/json"},"hooks":{"init":[],"beforeRequest":[],"beforeRedirect":[],"beforeRetry":[],"beforeError":[],"afterResponse":[]},"decompress":false,"throwHttpErrors":true,"followRedirect":true,"isStream":false,"responseType":"json","resolveBodyOnly":false,"maxRedirects":10,"prefixUrl":"","methodRewriting":true,"ignoreInvalidCookies":false,"http2":false,"allowGetBody":false,"rejectUnauthorized":true,"pagination":{"countLimit":null,"requestLimit":10000,"stackAllItems":true},"pathname":"/admin/api/2019-10/locations.json","hostname":"james-jumping-jeans.myshopify.com","protocol":"https:","username":"","password":"","url":"https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json","timeout":{"request":60000}}
"GET / HTTP/1.1\r\nuser-agent: shopify-api-node/3.3.1\r\nx-shopify-access-token: redacted\r\naccept: application/json\r\nHost: james-jumping-jeans.myshopify.com\r\nConnection: close\r\n\r\n"

szmarczak commented 4 years ago

Nice. So it really sent a request to / If you can provide the following that would be awesome:

response.url
response.requestUrl
response.redirectUrls

szmarczak commented 4 years ago

What does the uri argument look like?

jbgrunewald commented 4 years ago

@szmarczak Here you go,

response.req._header: "GET / HTTP/1.1\r\nuser-agent: shopify-api-node/3.3.1\r\nx-shopify-access-token: redacted\r\naccept: application/json\r\nHost: james-jumping-jeans.myshopify.com\r\nConnection: close\r\n\r\n"

response.url: "https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json"

response.requestUrl: "https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json"

response.redirectUrls: []

szmarczak commented 4 years ago

I think I know what's going on. The only way I can reproduce this, is when options.path is directly set to undefined:

const uri = {
    pathname: '/admin/api/2019-10/locations.json',
    hostname: 'james-jumping-jeans.myshopify.com',
    protocol: 'https:',
    path: undefined
};

const response = await got(uri, {retry: 0});

console.log(response.req._header); // GET / HTTP/1.1
console.log(response.url); // https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json
console.log(response.requestUrl); // https://james-jumping-jeans.myshopify.com/admin/api/2019-10/locations.json
console.log(response.redirectUrls); // []

Can you pass path: '/admin/api/2019-10/locations.json' instead of pathname and log the above again (response.req._header, response.url, response.requestUrl, response.redirectUrls)?

szmarczak commented 4 years ago

Hello? Is the issue fixed? :thinking:

jbgrunewald commented 4 years ago

No, but I have an alternative solution that will suffice for my needs. Unfortunately I can’t commit more time investigating this issue and the issue doesn’t seem to be reproducible for others.

James Grunewald

On May 15, 2020, at 12:46 PM, Szymon Marczak notifications@github.com wrote:

Hello? Is the issue fixed? 🤔

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

szmarczak commented 4 years ago

I'm 100% sure you were defining uri.path as undefined, because that's the only possible way. http2-wrapper (the underlying HTTP2 solution written by me) is more lenient than the Node.js native, that's why it works.

Got 12 is going to have legacy Url options support dropped and will throw on such. It's much safer to use a string or a URL instance instead.

lpinca commented 4 years ago

I'm going to reopen this as it seems this issue is affecting other users https://github.com/MONEI/Shopify-api-node/issues/370#issuecomment-629296707.

lpinca commented 4 years ago

@satyavh can you create a reproducible test case?

Jtosbornex commented 4 years ago

I just wanted to add to the conversation that I am getting all of my requests timing out. I think its related to #370 because when I downgraded to 2.25.1 my requests seem to be working.

lpinca commented 4 years ago

Me and @szmarczak need a way to reproduce the issue. If any of you can create a reproducible test case and share it, it would be great.

szmarczak commented 4 years ago

@lpinca Can you try releasing an alpha/beta with got@11.1.4?

lpinca commented 4 years ago

I could but I have to use a beta of nock, was waiting for a stable release.

lpinca commented 4 years ago

Oh nvm it seems you fixed the issues with nock. Will cut a new release now.

lpinca commented 4 years ago

Done. @satyavh @Jtosbornex @pcross616 and @jbgrunewald (if you are still using got) can you try if version 3.3.2 fixes the issue?

MONEI / Shopify-api-node

Support for Configurable Request Library #371