skip http patching

[xavdid]

@reganstarr ran into an issue where our changing of the core http module caused the AWS SDK to stop working as expected. this allows the lambda handler to be created without patching http. Currently this works for tests (where the handler is created directly) but not in the actual running of the app (which i'll need to think on a little more). Will probably have to add something to the schema.

Note that i wasn't able to get the tests working. While normal requires go use a cache (such as require('lodash'), core modules (like http) don't. Additionally, all core requires share an instance and (as far as I can find) there's not a way around that. A quick example:

function requireUncached(module){
    delete require.cache[require.resolve(module)]
    return require(module)
}

require.cache // => {}

const l = require('lodash')
const l2 = require('lodash')

Object.keys(require.cache) // => [ '/Users/david/projects/zapier/platform-core/node_modules/lodash/lodash.js' ]
l === l2 // => true
const l3 = requireUncached('lodash')
l === l3 // => false

// new session

const h = require('http')
require.cache // => {}
const h2 = require('http')
h === h2 // => true
const h3 = requireUncached('http')
h === h3 // => true

Since core modules don't use the cache, I can't find a way to get a "clean" copy. I think long-term it'd be nice to not modify the core module (that seems like a no-no) but being able to log all http requests is certainly nice. @stevelikesmusic do you have any ideas?

[stevelikesmusic]

@xavdid maybe it's me not understanding the problem that patching http is trying to solve. Why aren't we just handing logging within request-client or even better, within our wrapped fetch? I agree, patching http seems like dangerous waters. We only use our request-client or request-client-internal to make all requests, right? We never create a request directly with http(s). And we say z.request needs to be used to get logging?

Regarding staying with the patching approach, was AWS giving any errors or logging as to what was happening? Or just, requests weren't working?

[xavdid]

Yeah, great question(s)!

The core problem this solves is that we log every request to GL, regardless of how the dev makes it. This code specifically deals with non-z.request requests. We bail early if it's us:

zapier-platform-core/src/tools/create-http-patch.js

Lines 46 to 49 in 5e8ebba

	// Ignore requests made via the request client
	if (_.get(options.headers, 'user-agent', []).indexOf('Zapier') !== -1) {
	return originalRequest(options, callback);
	}

it's nice because even if a dev is using one of the many other request libs out there, we still get to see the logs, which makes debugging and supporting dramatically easier.

In AWS's case, the request went out but the response was empty every time (since something in the root callback wasn't actually getting called). There's more info in this thread but the fact that it worked on node 10 makes me think that our patch is going to bust soon anyway.

If we merge this now, we allow devs to opt out of the patch in a non-breaking way. It's a little confusing because they have to make sure to pass the new option every time they call createAppTester, since any un-optioned call can poison the pot.

Then, when AWS releases node 10 we can re-evaluate. I'd be willing to bet that the bugs aren't worth getting logs for the probably (very small) number of devs who make requests outside z.request.

[stevelikesmusic]

I agree @xavdid! That makes sense. Let's go with giving an out for skipping the patch for now. Long-term, we can steer everyone towards z.request and augment it if needed.

I am curious as to what's causing the response to come back empty though...

[xavdid]

I mean the very reasonable use case that I had mostly spaced until just now is that they're using an SDK of some sort and we want longs on those calls too. We'll look closer once node 10 is ready to be looked at. It doesn't sound like there's any way to guarantee that we get a clean copy of a core module. I'll just write docs that are clear you have to always pass the option.

I'll need to think on how to use this when the app runs for real (outside tests) and once that's ready i'll make this pr non-draft. Thanks a bunch!

[stevelikesmusic]

Great @xavdid.

Just for clarity, are the failing tests on the core side, or on a CLI integration's test suite?

[xavdid]

the tests I added in this PR don't work (unless only the new functionality is tested) because any other test that calls appTester(... patches http. So if you run only the one assertion it works, but not when part of the suite.

[xavdid]

ok, users should be able to store data on their root app and we'll pick it up in the wrapper. should safely exit if something goes wrong. Also, godzilla doesn't use the app wrapper, so this is safe there.

[stevelikesmusic]

Great @xavdid! I like the idea of setting this within the definition—nice.

I do wonder if we should drop app from appFlags or call it simply options. Not to put too much weight on the name, but it could be a pain if we want to change it later.

[stevelikesmusic]

Can we call it just flags or options? Having app in the name feels redundant.

[stevelikesmusic]

So this will opt out of logging HTTP requests (even z.request), right? Though, it would be strange to skipHttpPatch and still use z.request.

[xavdid]

z.request logs separately, so this'll opt out of logging made by anything besides z.request. namely, 3rd party SDKs