improvement(core): include user-agent for internal rpc calls

[xavdid]

This helps us identify these calls internally. Hopefully will help prevent things like this.

Also cleaned up some promise code while I was in there.

[xavdid]

I'm OOO friday, so if this looks good and you still plan on cutting 9.4.0, then kindly merge this before you release.

also given that there's no API-level changes (that is, the surface of the code didn't change at all), it's probably better as 9.3.1. NBD either way though!

[xavdid]

sorting

[eliangcs]

I have a suggestion about the User-Agent header format. If you agree, feel free to merge once you adopt the suggested change. If you don't, let me know what you think.

also given that there's no API-level changes (that is, the surface of the code didn't change at all), it's probably better as 9.3.1.

There's one actually: #195 adds an allowGetBody option to the z.request API. I kind of consider it a public API change, even though we don't encourage using it.

I'll hold on to 9.4.0 release until this PR is merged.

[eliangcs]

Should we make the format consistent with the CLI?

zapier-platform/packages/cli/src/utils/api.js

Line 67 in cfb1205

'User-Agent': `${constants.PACKAGE_NAME}/${constants.PACKAGE_VERSION}`,

I'd also suggest removing the (via node-fetch) part. Two reasons: 1) zapier-platform-core/<version> alone can already help us infer the underlying HTTP client library we use. 2) Minor, but it's easy to forget to update it if we were to replace node-fetch with something else.

So the suggested change would be:

Suggested change

	'user-agent': `${PACKAGE_NAME} v${PACKAGE_VERSION} (via node-fetch)`,
	'user-agent': `${PACKAGE_NAME}/${PACKAGE_VERSION}`,

[xavdid]

Yeah, that's fair! You're 👍 to merge and cut 9.4.0 if you'd like. We can also wait until week after next if we think we'll have something to release from the retreat. Your call!

On Thu, Apr 23, 2020 at 11:27 PM Chang-Hung Liang ***@***.***> wrote: ***@***.**** approved this pull request. I have a suggestion about the User-Agent header format. If you agree, feel free to merge once you adopt the suggested change. If you don't, let me know what you think. also given that there's no API-level changes (that is, the surface of the code didn't change at all), it's probably better as 9.3.1. There's one actually: #195 <#195> adds an allowGetBody option to the z.request API. I kind of consider it a public API change. I'll hold on to 9.4.0 release until this PR is merged. ------------------------------ In packages/core/src/tools/request-client-internal.js <#204 (comment)> : > }; // Return our INTERNAL convenient flavor of resp. No middleware! -const request = options => { - const fetchUrl = options.url; - options = _.omit(options, 'url'); - - return fetch(fetchUrl, options) - .then(resp => { - resp.options = options; - return resp; - }) - .then(parseResponse); +const request = async options => { + const { url: fetchUrl, ...fetchOptions } = options; + + fetchOptions.headers = { + 'user-agent': `${PACKAGE_NAME} v${PACKAGE_VERSION} (via node-fetch)`, Should we make the format consistent with the CLI? https://github.com/zapier/zapier-platform/blob/cfb1205a20bffff624425f97d14b5728fc6bd8b8/packages/cli/src/utils/api.js#L67 I'd also suggest removing the (via node-fetch) part. Two reasons: 1) zapier-platform-core/<version> alone can already help us infer the underlying HTTP client library we use. 2) Minor, but it's easy to forget to update it if we were to replace node-fetch with something else. So the suggested change would be: ⬇️ Suggested change - 'user-agent': `${PACKAGE_NAME} v${PACKAGE_VERSION} (via node-fetch)`, + 'user-agent': `${PACKAGE_NAME}/${PACKAGE_VERSION}`, — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#204 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMYP2F5OG7CWDSL62AZYTROEWNXANCNFSM4MPTFLLQ> .

-- ~David Brownman Senior Platform Engineer | Zapier <https://zapier.com> c: 303.819.0850 tw: @xavdid