Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make hooks better 🦄 #523

Closed
wants to merge 3 commits into from
Closed

Make hooks better 🦄 #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2276e0
Make hooks better 🦄
szmarczak Jul 15, 2018
6c710ad
fix `got.hooks` alias
szmarczak Jul 15, 2018
b22c3ab
fix some bugs
szmarczak Jul 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions source/create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
'use strict';
const URLGlobal = typeof URL === 'undefined' ? require('url').URL : URL; // TODO: Use the `URL` global when targeting Node.js 10
const extend = require('extend');
const errors = require('./errors');
const assignOptions = require('./assign-options');
const asStream = require('./as-stream');
const asPromise = require('./as-promise');
const normalizeArguments = require('./normalize-arguments');
const deepFreeze = require('./deep-freeze');
const defineConstProperty = require('./define-const-property');

const makeNext = defaults => (path, options) => {
let url = path;
Expand All @@ -24,6 +25,8 @@ const makeNext = defaults => (path, options) => {
};

const create = defaults => {
defaults = extend(true, {}, defaults);

const next = makeNext(defaults);
if (!defaults.handler) {
defaults.handler = next;
Expand Down Expand Up @@ -51,18 +54,19 @@ const create = defaults => {
return defaults.handler(url, options, next);
};

got.hooks = defaults.options.hooks = { // eslint-disable-line no-multi-assign
beforeRequest: [],
onSocketConnect: [],
onAbort: [],
...(defaults.options.hooks || {})
};

for (const method of defaults.methods) {
got[method] = (url, options) => got(url, {...options, method});
got.stream[method] = (url, options) => got.stream(url, {...options, method});
}

Object.assign(got, errors);
Object.defineProperty(got, 'defaults', {
value: deepFreeze(defaults),
writable: false,
enumerable: true,
configurable: true
});
defineConstProperty(got, {...errors, defaults}, true, ['defaults.options.hooks']);

return got;
};
Expand Down
12 changes: 0 additions & 12 deletions source/deep-freeze.js
View file
Open in desktop

This file was deleted.

25 changes: 25 additions & 0 deletions source/define-const-property.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';
const is = require('@sindresorhus/is');

module.exports = (where, properties, deep, excluded) => {
const deepFreeze = (obj, parent) => {
for (const [key, value] of Object.entries(obj)) {
const name = parent + '.' + key;

if (is.object(value) && !excluded.includes(name)) {
deepFreeze(obj[key], name);
}
}

return excluded.includes(parent) ? obj : Object.freeze(obj);
};

for (const [key, value] of Object.entries(properties)) {
Object.defineProperty(where, key, {
value: deep ? deepFreeze(value, key) : Object.freeze(value),
writable: false,
enumerable: true,
configurable: true
});
}
};
3 changes: 0 additions & 3 deletions source/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@ const defaults = {
throwHttpErrors: true,
headers: {
'user-agent': `${pkg.name}/${pkg.version} (https://github.com/sindresorhus/got)`
},
hooks: {
beforeRequest: []
}
}
};
Expand Down
23 changes: 6 additions & 17 deletions source/normalize-arguments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const urlToOptions = require('./url-to-options');
const isFormData = require('./is-form-data');

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

module.exports = (url, options, defaults) => {
if (Reflect.has(options, 'url') || (is.object(url) && Reflect.has(url, 'url'))) {
Expand Down Expand Up @@ -187,24 +186,14 @@ module.exports = (url, options, defaults) => {
delete options.timeout;
}

if (is.nullOrUndefined(options.hooks)) {
options.hooks = {};
}
if (is.object(options.hooks)) {
for (const hookEvent of knownHookEvents) {
const hooks = options.hooks[hookEvent];
if (is.nullOrUndefined(hooks)) {
options.hooks[hookEvent] = [];
} else if (is.array(hooks)) {
hooks.forEach(
(hook, index) => {
if (!is.function_(hook)) {
throw new TypeError(
`Parameter \`hooks.${hookEvent}[${index}]\` must be a function, not ${is(hook)}`
);
}
for (const [hookEvent, hooks] of Object.entries(options.hooks)) {
if (is.array(hooks)) {
Copy link
Contributor

@jstewmon jstewmon Jul 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails to validate the known hooks if something other than an array is provided, which will result in an error upstream.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. https://github.com/szmarczak/got/blob/b22c3abbe8daf905aae4bed66cddfc7b9ed72170/source/normalize-arguments.js#L191-L199

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks - I got tripped up reviewing the diff.

for (const [index, hook] of Object.entries(hooks)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not equivalent to the current implementation. Consider these examples:

const a = [1, 2];
a.foo = 'bar';
Object.entries(a); // [ [ '0', 1 ], [ '1', 2 ], [ 'foo', 1 ] ]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a bug. It's a feature!

if (!is.function(hook)) {
throw new TypeError(`Parameter \`hooks.${hookEvent}[${index}]\` must be a function, not ${is(hook)}`);
}
);
}
} else {
throw new TypeError(`Parameter \`hooks.${hookEvent}\` must be an array, not ${is(hooks)}`);
}
Expand Down
18 changes: 12 additions & 6 deletions source/request-as-event-emitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ const {CacheError, UnsupportedProtocolError, MaxRedirectsError, RequestError} =
const getMethodRedirectCodes = new Set([300, 301, 302, 303, 304, 305, 307, 308]);
const allMethodRedirectCodes = new Set([300, 303, 307, 308]);

const callAll = async (array, ...args) => {
for (const func of array) {
await func(...args); // eslint-disable-line no-await-in-loop
}
};

module.exports = (options = {}) => {
const emitter = new EventEmitter();
const requestUrl = options.href || (new URLGlobal(options.path, urlLib.format(options))).toString();
Expand Down Expand Up @@ -122,8 +128,9 @@ module.exports = (options = {}) => {

cacheReq.once('request', req => {
let aborted = false;
req.once('abort', _ => {
req.once('abort', () => {
aborted = true;
callAll(options.hooks.onAbort);
});

req.once('error', error => {
Expand All @@ -150,7 +157,9 @@ module.exports = (options = {}) => {

const socket = req.connection;
if (socket) {
const onSocketConnect = () => {
const onSocketConnect = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the origin of this conditional block are, but I think the right way to handle this event is to remove the if block and make this:

req.on('socket', () => { ... });

https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_event_socket

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, since onSocketConnect and onAbort should be attached to the request, I don't think these are sensible hooks for got to configure directly. If you want to configure these for a got instance, you really just need to configure a listener for request and attach the request event listeners in the request listener.

Copy link
Collaborator Author

@szmarczak szmarczak Jul 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the origin of this conditional block are, but I think the right way to handle this event is to remove the if block

#429

Actually, since onSocketConnect and onAbort should be attached to the request, I don't think these are sensible hooks for got to configure directly. If you want to configure these for a got instance, you really just need to configure a listener for request and attach the request event listeners in the request listener.

Why would you do the same thing twice? Oh wait. I do that twice. I'll change that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reference to #429. Maybe I'm missing something, but I don't think that was the right fix b/c if req.connection is just testing a race condition. Shouldn't it be:

req.once('socket', (sock) => {
  sock.once('connect', conn => {
    // ...
  }
};

I can see the convenience of low-friction hooks. Maybe there's a way to make the configuration a sort of DSL that got can use to wire these up. Something like this:

{
  events: {
    request: {
      ['on|once']: {
        socket: {
          null: [(sock) => { console.log('request.on(socket)') }],
          connect: [(conn) => { console.log('socket.on(connect)') }]
        }
      },
    }
  }
}

With the above, the configuration can be traversed and the listeners can be subscribed without having to add handling code to got for every event.

I used null as a sentinel value to mean the subscriber for the event as opposed to a child subscription. Something else (symbol?) would work also.

Copy link
Collaborator Author

@szmarczak szmarczak Jul 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something, but I don't think that was the right fix b/c if req.connection is just testing a race condition. Shouldn't it be:

I don't know. Can you make another issue for that?

Copy link
Collaborator Author

@szmarczak szmarczak Jul 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the convenience of low-friction hooks.

Great! We'll discuss this later. I was gonna send a PR with rewrited hooks from scratch but I decided current implementation is better. :)

await callAll(options.hooks.onSocketConnect);

const uploadEventFrequency = 150;

progressInterval = setInterval(() => {
Expand Down Expand Up @@ -241,10 +250,7 @@ module.exports = (options = {}) => {
options.headers['content-length'] = uploadBodySize;
}

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

get(options);
} catch (error) {
Expand Down
2 changes: 1 addition & 1 deletion test/arguments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ test('throws TypeError when known `hooks` array item is not a function', async t
});

test('allows extra keys in `hooks`', async t => {
await t.notThrows(() => got(`${s.url}/test`, {hooks: {extra: {}}}));
await t.notThrows(() => got(`${s.url}/test`, {hooks: {extra: []}}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this test is to show that extra keys are not required to pass validation b/c they should be ignored. The test title could be more clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would someone assign non-hook object to hooks? What's the use case? What big problem does this solve? Sorry, but I don't see that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It allows for encapsulation. Here's a contrived example:

class GotHooks {

  constructor () {
    this.message = 'Encapsulated Got Hooks';
    this.beforeRequest = [
      () => this.runRook()
    ];
  }

  async runRook() {
    console.log(this.message);
  }
}

If got validates only the properties it is concerned with, then everything is fine. If got extraneously demands that the object only have properties that correspond to known hooks, then the user has to jump through some hoops to create an object that accomplishes the same thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed response, I really appreciate that! 👍

});

test.after('cleanup', async () => {
Expand Down
129 changes: 62 additions & 67 deletions test/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,97 +7,92 @@ let s;

test.before('setup', async () => {
s = await createServer();
const echoHeaders = (req, res) => {
s.on('/', async (req, res) => {
await delay(500);
res.statusCode = 200;
res.write(JSON.stringify(req.headers));
res.end();
};
s.on('/', echoHeaders);
res.end(JSON.stringify(req.headers));
});
await s.listen(s.port);
});

test('beforeRequest receives normalized options', async t => {
await got(
s.url,
{
json: true,
hooks: {
beforeRequest: [
options => {
t.is(options.path, '/');
t.is(options.hostname, 'localhost');
}
]
}
await got(s.url, {
json: true,
hooks: {
beforeRequest: [
options => {
t.is(options.path, '/');
t.is(options.hostname, 'localhost');
}
]
}
);
});
});

test('beforeRequest allows modifications', async t => {
const res = await got(
s.url,
{
json: true,
hooks: {
beforeRequest: [
options => {
options.headers.foo = 'bar';
}
]
}
const res = await got(s.url, {
json: true,
hooks: {
beforeRequest: [
options => {
options.headers.foo = 'bar';
}
]
}
);
});
t.is(res.body.foo, 'bar');
});

test('beforeRequest awaits async function', async t => {
const res = await got(
s.url,
{
json: true,
hooks: {
beforeRequest: [
async options => {
await delay(100);
options.headers.foo = 'bar';
}
]
}
const res = await got(s.url, {
json: true,
hooks: {
beforeRequest: [
async options => {
await delay(100);
options.headers.foo = 'bar';
}
]
}
);
});
t.is(res.body.foo, 'bar');
});

test('beforeRequest rejects when beforeRequest throws', async t => {
await t.throws(
() => got(s.url, {
hooks: {
beforeRequest: [
() => {
throw new Error('oops');
}
]
}
}),
{
instanceOf: Error,
message: 'oops'
await t.throws(got(s.url, {
hooks: {
beforeRequest: [
() => {
throw new Error('oops');
}
]
}
);
}), {message: 'oops'});
});

test('beforeRequest rejects when beforeRequest rejects', async t => {
await t.throws(
() => got(s.url, {
hooks: {
beforeRequest: [() => Promise.reject(new Error('oops'))]
}
}),
{
instanceOf: Error,
message: 'oops'
await t.throws(got(s.url, {
hooks: {
beforeRequest: [() => Promise.reject(new Error('oops'))]
}
);
}), {message: 'oops'});
});

test('extend got + onAbort hook', async t => {
let aborted = false;

const extended = got.extend();
extended.hooks.onAbort.push(() => {
aborted = true;
});

const p = extended(s.url);
p.cancel();

await t.throws(p);
await delay(200); // Wait because it may throw before the hook is called

t.is(aborted, true);
});

test.after('cleanup', async () => {
Expand Down