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.

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

Set default value for an options object #1495

Merged
merged 1 commit into from Oct 9, 2020

Conversation

mrmlnc
Copy link
Contributor

@mrmlnc mrmlnc commented Oct 9, 2020

Hello,

I ran into a problem that after the client extension by the .extend method, the init hook gets an undefined instead of an object as options.

The following example illustrates the problem.

const got = require('got').default;

const client = got.extend({
	hooks: {
		init: [
			(options) => {
				console.dir({ options }, { colors: true });
			}
		]
	}
});

(async () => {
	await client('https://google.com'); // { options: undefined }
	await client('https://google.com', {}); // { options: {} }
	await client({ url: 'https://google.com' }); // { options: { url: <value> }}
})();

Looks like a bug, because I see that the init hook should get an Options object according to the signature in the types.

export type InitHook = (options: Options) => void;

So, this PR fixes the problem. If you accept this change, I can make a backport to the v11 branch.

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates in both the README and the types.

@mrmlnc
Copy link
Contributor Author

mrmlnc commented Oct 9, 2020

Looks like CI fails due to last update of node.js. Works fine under 12.18.4 and breaks after 12.19.0.

  Uncaught exception in test/timeout.ts

  Error [ERR_INTERNAL_ASSERTION]: This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
  Please open an issue with this stack trace at https://github.com/nodejs/node/issues


  › Please open an issue with this stack trace at https://github.com/nodejs/node/issues
  › assert (internal/assert.js:14:11)
  › TCP.onStreamRead (internal/stream_base_commons.js:188:23)

@szmarczak szmarczak merged commit 390b145 into sindresorhus:master Oct 9, 2020
@szmarczak
Copy link
Collaborator

@mrmlnc Can you please open another issue for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants