Add `beforeRequest` hook #516

jstewmon · 2018-07-11T18:26:12Z

This PR build on the leverage of being able to create got instances (thanks @knksmith57!) by adding a finalize option, which eases the use of HMAC request signatures, such as AWS V4.

I hope the updated example in the readme makes the motivation clear.

I explored implementing this using an event, but it works better if the hook can be an async function (also demonstrated in the readme).

Along the way, I made a couple of tweaks to other areas of the code, which I included here in separate commits. I can pull those off if they're not wanted, but I thought they were small enough that it would just be easier to propose them alongside this new feature.

szmarczak · 2018-07-11T18:42:52Z

Note: creating got instances is still in the works.

Seems good, but I don't see any sense of that. You could just do:

const sign = ...;

const instance = got.create({
	options: got.defaults.options,
	methods: got.defaults.methods,
	handler: (url, options, next) => {
		options.headers['sign'] = sign(options);

		return next(url, options);
	}
});

Along the way, I made a couple of tweaks to other areas of the code, which I included here in separate commits.

Can you do another PR for that? To make everything more clear :)

szmarczak · 2018-07-11T18:48:37Z

source/request-as-event-emitter.js

@@ -228,6 +228,9 @@ module.exports = (options = {}) => {
 				options.headers['content-length'] = uploadBodySize;
 			}

+			if (is.function(options.finalize)) {
+				await options.finalize(options);
+			}


If got.create wasn't existing, then it'd be very helpful. I think this option is useless.

I'm sorry, but we are completely misunderstanding each other.

If I understand your suggestion regarding a handler, then a custom handler would run before the default handler.

The entire point of this feature is to create an opportunity to add a signature after the request has been normalized, just before it is sent.

Here's a little test that might illustrate the difference:

test('does something useful', async t => { const instance = got.create({ baseUrl: `${s.url}/api/`, options: got.defaults.options, methods: got.defaults.methods, handler: (url, options, next) => { t.is(options.hostname, 'localhost') t.is(options.path, '/api/?foo=bar') return next(url, options); } }); await instance('?foo=bar'); });

result:

1 test failed does something useful /Users/jstewmon/dev/forks/got/test/finalize.js:95 94: handler: (url, options, next) => { 95: t.is(options.hostname, 'example.com') 96: t.is(options.path, '/api/?foo=bar') Difference: - undefined + 'example.com' Object.handler (test/finalize.js:95:6) got (source/create.js:34:20) test/finalize.js:100:8

Thanks for clarifying this out :)

This should be .on('beforeRequest', ...). Options may be overridden.

@szmarczak We could handle that while merging options if needed. if beforeRequest is set and we're merging in options with another beforeRequest, we just wrap it in a function that calls both.

I don't like events that allow mutation.

I considered doing that in assignOptions, but I decided that it was better to leave that choice to the caller, since I think it is pretty easy:

const got = require('got'); const a = got.extend({beforeRequest: opts => { opts.header.foo = 'bar'; }}); const b = a.extend({beforeRequest: opts => { a.defaults.beforeRequest(opts); opts.header.bar = 'foo'; }});

If the functions are chained, I don't think there's a convenient way to replace the beforeRequest option if that's the behavior one desires.

we just wrap it in a function that calls both.

@sindresorhus We need to be strict here. Only for beforeRequest and retries.

I decided that it was better to leave that choice to the caller, since I think it is pretty easy

It's OK for now. Lots of things is gonna change if #510 gets merged. #510 needs a lot of polishing.

I took a look at #510 and had the same initial reaction as others - I don't get it. :-)

Personally, I don't see providing merge/extend features as creating a lot of leverage. Since the options are accessible on a got instance, the caller can always use a merge strategy of their choosing to create the configuration for a new client. If it were my choice, I'd probably just provide Object.assign({}, old, new) semantics for creating new instances.

@jstewmon It's not as easy as it seems to be ;) That's for the future: Got plugins.

sindresorhus · 2018-07-11T18:55:49Z

readme.md

@@ -243,6 +243,15 @@ Determines if a `got.HTTPError` is thrown for error responses (non-2xx status co

 If this is disabled, requests that encounter an error status code will be resolved with the `response` instead of throwing. This may be useful if you are checking for resource availability and are expecting error responses.

+###### finalize


I think beforeRequest would be a clearer name.

I'm ok with that. I chose finalize to indicate that this is the very last thing that happens before the request is sent.

I'll rename.

sindresorhus · 2018-07-11T20:25:00Z

test/before-request.js

+		{
+			json: true,
+			beforeRequest: async options => {
+				return new Promise(


Use the delay module (we already have it as a devDependency here).

Ah, I didn't see it. Thanks and done!

jstewmon · 2018-07-11T21:17:28Z

readme.md

+Type: `Function`<br>
+Default: `undefined`
+
+Called with the normalized request options just before the request is sent. You can modify the object. This is especially useful in conjunction with [`got.extend()`](#instances) and [`got.create()`](advanced-creation.md) when you want to create an API client that uses HMAC-signing.


Is it worth noting that changing the body is not recommended, since the content-length has already been set?

brandon93s · 2018-07-11T21:23:37Z

This is similar in objective to interceptors in axios. It's also a great place to add support for plugins, or hooks. An API like the following would allow us to expand this to different hooks and provide support for multiple hooks at once:

const plugin = got.hooks.beforeRequest.add(async req => {

});

got.hooks.beforeRequest.remove(plugin);

Hooks here could be things like:

Authentication
Logging / Debugging
- For example: chalk() of network activity in development mode
Etc

I support the current implementation, but wanted to bring up the alternative above if we want to future proof the implementation.

jstewmon · 2018-07-11T21:35:28Z

I actually ended up coming back to got for this after trying to solve the problem with axios interceptors. :-)

axios request interceptors run before their dispatchRequest method, which is where they normalize the request.

But, that's not your point...

I considered allowing the config setting an array or a function, but I decided that it was best to start the conversation around the feature by making it as simple as possible.

If we allow the setting to be an array (without providing a custom interface), I think that might provide better mechanics for anyone wanting to apply a custom merge strategy to the options for a new instance (versus the function wrapping discussed earlier).

If it's conceivable that additional hook points would be added in the future, then we might consider having a top-level hooks setting, which is an object where they keys are the hook names and the values are Array|Function.

szmarczak · 2018-07-12T06:47:28Z

@brandon93s +1. ~~~But how do hooks compare to EventEmitter? What's the advantage?~~~

sindresorhus · 2018-07-12T12:55:16Z

@szmarczak Events are meant as one-way notifications. Hooks let us transform.

jstewmon · 2018-07-12T16:48:43Z

I rebased off master and updated the interface to reflect the discussion about future-proofing the configuration interface for additional hook events.

szmarczak · 2018-07-13T16:31:52Z

readme.md

-}
-
-request(`https://${config.host}/production/users/1`);
+// Create a Got instance to use relative paths and signed requests


This is what I wanted! A real example :) Now I know the advantage of beforeRequest. It's always before request. No need to merge anything (you could, but you now, if you merge two merged groups the order is not what you wanted, so beforeRequest does that job 🎉 ) Cool 🦄

szmarczak · 2018-07-13T16:33:52Z

readme.md


-request(`https://${config.host}/production/`, {
-	// All usual `got` options
+const response = await awsClient('/endpoint/path', {


Replace:

const response = await awsClient('/endpoint/path', { // Request-specific options });

With:

const response = await awsClient('/endpoint/path', options);

@szmarczak I don't think we should add variables that are not defined in the example. Ideally, examples should be copy-paste runnnable. I also think the comment helps make it clearer.

Okay then :)

szmarczak · 2018-07-13T16:39:35Z

source/request-as-event-emitter.js

@@ -241,6 +241,11 @@ module.exports = (options = {}) => {
 				options.headers['content-length'] = uploadBodySize;
 			}

+			for (const hook of options.hooks.beforeRequest) {
+				// eslint-disable-next-line no-await-in-loop


I would do: await hook(options); // eslint-disable-line no-await-in-loop

@szmarczak Let's try not to nitpick too much. The super minor stuff we can just fix ourselves when merging.

I just want to note that we should keep the style of coding. That's all.

szmarczak · 2018-07-13T16:49:42Z

source/index.js

@@ -23,6 +23,9 @@ const defaults = {
 		throwHttpErrors: true,
 		headers: {
 			'user-agent': `${pkg.name}/${pkg.version} (https://github.com/sindresorhus/got)`
+		},
+		hooks: {
+			beforeRequest: []


I just want to note: got.create({options: {}, methods: [], handler: () => {}}) shouldn't throw. It'd be better if hooks were done like @brandon93s has suggested:

An API like the following would allow us to expand this to different hooks and provide support for multiple hooks at once:

const plugin = got.hooks.beforeRequest.add(async req => { }); got.hooks.beforeRequest.remove(plugin);

Hooks here could be things like:

Authentication

Logging / Debugging

For example: chalk() of network activity in development mode

Etc

I strongly disagree. I'll repeat the rationale I provided earlier in the discussion:

If we allow the setting to be an array (without providing a custom interface), I think that might provide better mechanics for anyone wanting to apply a custom merge strategy to the options for a new instance (versus the function wrapping discussed earlier).

Requiring the use of a custom interface adds no value and prohibits advanced configuration scenarios.

If we allow the setting to be an array (without providing a custom interface)

OK. But this doesn't change anything.

Requiring the use of a custom interface adds no value and prohibits advanced configuration scenarios.

I disagree. What advanced configuration scenarios does it prohibit?

I think that might provide better mechanics for anyone wanting to apply a custom merge strategy to the options for a new instance (versus the function wrapping discussed earlier).

You're right here.

Let's assume there's got.hooks and handler receives a new argument: hooks

{ options, methods, handler: (options, hooks, next) => ... }

What about that? Maybe that's just me, probably I'm wrong. I just haven't seen that from your side.

BTW, do we really need to specify empty hooks in the defaults?

I prefer just a simple array too.

@szmarczak Making it just an array enables the user to construct it however they want. They can, for example, order it based on a condition.

Same reason we have:

got(..., { headers: {foo: true} });

Instead of:

got.headers.add({foo: true});

Ah, I see your point now...

I suspect that there is a performance advantage to defining the defaults.

Without the defaults, an object and knownHookEvents.length arrays will be created for every request, resulting in more work for the garbage collector.

Further, I think that dynamically creating the properties will introduce a new hidden class for every request. So, I think it is best to keep the default defined.

These are certainly micro optimizations, but I don't see a disadvantage to defining the defaults. Do you?

I don't :) But look: there are other properties that get redefined: options.timeout, options.followRedirect, options.retry and more. I don't think it's a big deal.

The array approach does come with the caveat that we can't support addition or removal of hooks from an instance since the options are run through an Object.freeze. A new instance would have to be created to modify hooks - they must be provided at instantiation.

@brandon93s I would consider that a feature. Immutability is good, it prevents a lot of weird bugs.

Do you have any use-case for when you would want to modify hooks after instantiation?

No, nothing in particular comes to mind. I'm comfortable with how it has been implemented here. I prefer the consistent API over additional methods to maintain hooks.

szmarczak · 2018-07-13T17:46:17Z

source/normalize-arguments.js

@@ -8,6 +8,7 @@ const urlToOptions = require('./url-to-options');
 const isFormData = require('./is-form-data');

 const RETRY_AFTER_STATUS_CODES = new Set([413, 429, 503]);
+const knownHookEvents = ['beforeRequest'];


This should be named KNOWN_HOOK_EVENTS to keep the consistency of the code style :)

I saw RETRY_AFTER_STATUS_CODES was chosen recently in #508, but I'm wondering if y'all would like to reconsider that choice and make it retryAfterStatusCodes as was the alternative suggestion in that discussion.

This is the only place in this package where this naming style is used. I know that uppercase constants is conventional in some languages, but not JS. Further, having two styles for variable names requires a decision to be made about when to use the uppercase style. Is it for all vars declared as const? It is for const outside of a function?

@jstewmon I agree. I feel like I never know when to put it in uppercase. Better to just always use normal camelcase, indeed.

sindresorhus · 2018-07-13T17:50:40Z

readme.md

@@ -258,6 +258,24 @@ Determines if a `got.HTTPError` is thrown for error responses (non-2xx status co

 If this is disabled, requests that encounter an error status code will be resolved with the `response` instead of throwing. This may be useful if you are checking for resource availability and are expecting error responses.

+###### hooks
+
+Type: `Object<string, Array<Function>`<br>


This is missing >

sindresorhus · 2018-07-13T17:52:55Z

readme.md

+const awsClient = got.extend(
+	{
+		baseUrl: 'https://<api-id>.execute-api.<api-region>.amazonaws.com/<stage>/',
+		beforeRequest: async options => {


beforeRequest needs to be nested in hooks

sindresorhus · 2018-07-13T17:54:52Z

readme.md


-request(`https://${config.host}/production/`, {
-	// All usual `got` options
+const response = await awsClient('/endpoint/path', {


@szmarczak I don't think we should add variables that are not defined in the example. Ideally, examples should be copy-paste runnnable. I also think the comment helps make it clearer.

sindresorhus · 2018-07-13T17:56:27Z

source/request-as-event-emitter.js

@@ -241,6 +241,11 @@ module.exports = (options = {}) => {
 				options.headers['content-length'] = uploadBodySize;
 			}

+			for (const hook of options.hooks.beforeRequest) {
+				// eslint-disable-next-line no-await-in-loop


@szmarczak Let's try not to nitpick too much. The super minor stuff we can just fix ourselves when merging.

szmarczak · 2018-07-13T18:17:28Z

readme.md

+
+See the [AWS section](#aws) for an example.
+
+**Note**: Modifying the `body` is not recommended because the `content-length` header has already been computed and assigned.


Maybe just Object.freeze that?

There are a few ways this could be strictly prohibited, but I think this is case where it's best to provide guidance but not prohibit advanced usage. Someone may have a valid reason for modifying the body, so this note is here to hint they they probably need to reassign content-length if they change body.

I agree. Or maybe just move

https://github.com/jstewmon/got/blob/c0aa4d74b9d9d94c858520586b5def68440cd9a9/source/request-as-event-emitter.js#L246

right after try before it calculates the body size?

That would violate the expectation that got will make no further changes to the options before the request is sent. ;-)

That would violate the expectation that got will make no further changes to the options before the request is sent.

But that's only one header... I think that'd be fine :)

@sindresorhus What do you think?

If that is a signed header, it will break the whole thing. It's a big deal.

Oh, then you're right. You should note that too.

I updated to say that Got will make no further changes to the request before it is sent.

I used the signing feature as an example of why I did not want to move the call before the last attempt to set content-length, but we shouldn't specifically mention request signing in the docs for beforeRequest, since it's not tightly coupled to request signing.

szmarczak

About changing content-length header:

If that is a signed header, it will break the whole thing.

Can you note that? People will need to sign that again then.

szmarczak · 2018-07-13T19:39:51Z

readme.md

@@ -270,7 +270,7 @@ Hooks allow modifications during the request lifecycle. Hook functions may be as
 Type: `Array<Function>`<br>
 Default: `[]`

-Called with the normalized request options just before the request is sent. You can modify the object. This is especially useful in conjunction with [`got.extend()`](#instances) and [`got.create()`](advanced-creation.md) when you want to create an API client that uses HMAC-signing.
+Called with the normalized request options just before the request is sent. Got will make no further changes to the request before it is sent. This is especially useful in conjunction with [`got.extend()`](#instances) and [`got.create()`](advanced-creation.md) when you want to create an API client that uses HMAC-signing.


Butter butter. Don't repeat yourself ;)

just before the request is sent

it says that for itself :)

- hooks are required to be Array<Function> - normalize-arguments validates provided hooks and normalizes missing values

sindresorhus · 2018-07-14T12:14:55Z

This looks great now. Thank you so much for working on this, @jstewmon :)

This reverts commit 107756f.

jstewmon force-pushed the finalize branch from 9e5a72b to c9b5387 Compare July 11, 2018 18:29

szmarczak reviewed Jul 11, 2018

View reviewed changes

jstewmon force-pushed the finalize branch from c9b5387 to 3d4bbf3 Compare July 11, 2018 18:49

jstewmon mentioned this pull request Jul 11, 2018

Various minor code tweaks #517

Merged

sindresorhus reviewed Jul 11, 2018

View reviewed changes

jstewmon changed the title ~~add finalize option to ease request signing~~ add beforeRequest option to ease request signing Jul 11, 2018

sindresorhus reviewed Jul 11, 2018

View reviewed changes

sindresorhus changed the title ~~add beforeRequest option to ease request signing~~ Add beforeRequest option Jul 11, 2018

sindresorhus requested review from alextes and brandon93s July 11, 2018 20:29

jstewmon commented Jul 11, 2018

View reviewed changes

jstewmon force-pushed the finalize branch from 70fe061 to ad9fbb9 Compare July 12, 2018 16:47

szmarczak requested changes Jul 13, 2018

View reviewed changes

szmarczak reviewed Jul 13, 2018

View reviewed changes

sindresorhus requested changes Jul 13, 2018

View reviewed changes

sindresorhus changed the title ~~Add beforeRequest option~~ Add beforeRequest hook Jul 13, 2018

szmarczak reviewed Jul 13, 2018

View reviewed changes

jstewmon mentioned this pull request Jul 13, 2018

Standardize on camelcase #520

Merged

szmarczak requested changes Jul 13, 2018

View reviewed changes

szmarczak reviewed Jul 13, 2018

View reviewed changes

Jonathan Stewmon and others added 4 commits July 13, 2018 14:53

add finalize option to ease request signing

2a172a1

rename finalize to beforeRequest

ff50133

Update request-as-event-emitter.js

b8120c1

Update readme.md

3ff1a8a

Jonathan Stewmon and others added 8 commits July 13, 2018 14:53

use delay instead of Promise/timeout

a366b30

Update readme.md

310e819

add note warning not to change body in beforeRequest

e3882f7

move beforeRequest under hooks to support future hooks

a36c12f

- hooks are required to be Array<Function> - normalize-arguments validates provided hooks and normalizes missing values

fix: correct readme entries for hooks

002b914

fix: AWS example should use relative path in request

a5c9117

chore: document expectations of beforRequest hook

bc26cb7

chore: DRY beforeRequest docs

76b3611

jstewmon force-pushed the finalize branch from 738f593 to 76b3611 Compare July 13, 2018 19:54

brandon93s approved these changes Jul 13, 2018

View reviewed changes

sindresorhus merged commit 107756f into sindresorhus:master Jul 14, 2018

szmarczak added a commit to szmarczak/got that referenced this pull request Jul 15, 2018

Revert "Add beforeRequest hook (sindresorhus#516)"

780da1f

This reverts commit 107756f.

jstewmon deleted the finalize branch July 25, 2018 15:18

		@@ -243,6 +243,15 @@ Determines if a `got.HTTPError` is thrown for error responses (non-2xx status co

		If this is disabled, requests that encounter an error status code will be resolved with the `response` instead of throwing. This may be useful if you are checking for resource availability and are expecting error responses.

		###### finalize


		See the [AWS section](#aws) for an example.

		Note: Modifying the `body` is not recommended because the `content-length` header has already been computed and assigned.

Add beforeRequest hook #516

Add beforeRequest hook #516

Conversation

jstewmon commented Jul 11, 2018

szmarczak commented Jul 11, 2018

szmarczak Jul 11, 2018 • edited

Choose a reason for hiding this comment

jstewmon Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandon93s commented Jul 11, 2018

jstewmon commented Jul 11, 2018

szmarczak commented Jul 12, 2018 • edited

sindresorhus commented Jul 12, 2018 • edited

jstewmon commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szmarczak Jul 13, 2018 • edited

Choose a reason for hiding this comment

jstewmon Jul 13, 2018 • edited

Choose a reason for hiding this comment

szmarczak Jul 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Jul 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Jul 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szmarczak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Jul 14, 2018

Add `beforeRequest` hook #516

Add `beforeRequest` hook #516

szmarczak Jul 11, 2018 •

edited

jstewmon Jul 11, 2018 •

edited

sindresorhus Jul 11, 2018 •

edited

szmarczak commented Jul 12, 2018 •

edited

sindresorhus commented Jul 12, 2018 •

edited

szmarczak Jul 13, 2018 •

edited

jstewmon Jul 13, 2018 •

edited

szmarczak Jul 13, 2018 •

edited

sindresorhus Jul 13, 2018 •

edited

sindresorhus Jul 14, 2018 •

edited