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

Improve option normalization #1521

Closed
wants to merge 40 commits into from

Conversation

szmarczak
Copy link
Collaborator

@szmarczak szmarczak commented Nov 7, 2020

This allows us to normalize options on-the-fly. This will replace got.normalizeOptions / Request.normalizeOptions. The following should give a significant performance boost when called

import {Options} from 'got';
const options = new Options(); // or new Options({url: 'https://localhost'})

options.url = 'https://localhost';
options.url; //=> URL

options.url = 'invalid'; //=> throws

(async () => {
	for (let i = 0; i < 1000; i++) {
		await got(options);
	}
})();

Also it will be a lot easier to test the options.

TODO

  • Make some properties non-enumerable
  • Normalize Promise API arguments
  • Implement option merging
  • Implement defaults
  • Fix Promise API
  • Fix Got interface
  • Fix benchmarks
  • Invert option.methodRewriting mechanism (fixes Invert the methodRewriting option logic #1307)
  • Fix Got options -> HTTP(S) options Proxy handler. Missing has() and ownKeys().
  • ...

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.

@szmarczak
Copy link
Collaborator Author

szmarczak commented Nov 7, 2020

Of course it still will be possible to call got(url, {headers: {...}, ...}) or just got({headers: {...}, ...}).

@szmarczak

This comment has been minimized.

@szmarczak
Copy link
Collaborator Author

Also it will throw when passing a non-existing property on the Options. Migrate:

-got({token: 'asdf'});
+got({context: {token: 'asdf'}});

@szmarczak szmarczak changed the title Separate options Improve option normalization Nov 8, 2020
@szmarczak
Copy link
Collaborator Author

szmarczak commented Nov 8, 2020

It gives a significant performance boost in terms of normalization:

got - normalize options x 95,800 ops/sec ±2.19% (91 runs sampled)
got - new options x 202,692 ops/sec ±0.33% (96 runs sampled)

It's 2.1x as fast as got.normalizeOptions!

@szmarczak
Copy link
Collaborator Author

szmarczak commented Nov 9, 2020

Ok, so console.log(new Options()) also logged internals, which would be OK as the getters aren't logged by default. So I wanted to make this[INTERNALS].body, this[INTERNALS].json and this[INTERNALS].form not enumerable and the performance dropped back to 95k.

So instead I did make the entire INTERNALS not enumerable and it "kind of" solved the problem because the benchmarks now look like this:

got - normalize options x 95,929 ops/sec ±1.81% (94 runs sampled)
got - new options x 182,247 ops/sec ±0.72% (98 runs sampled)

1.9x is also great. But I wanted all the getters to get logged by default, but encountered this.

@szmarczak
Copy link
Collaborator Author

Also the Options template now requires to pass an ErrorType. This is to extract the types to a different file. In the end Got will export Options = Options<RequestError> anyway, so the types remain the same.

@szmarczak
Copy link
Collaborator Author

Ok, I've got the streams working, which is pretty nice!

https://github.com/szmarczak/got/blob/8798afc0c5f3ac4bbecde9e49c83cb2cb33722d6/demo.js

@sindresorhus
Copy link
Owner

Is this a breaking change?

@szmarczak
Copy link
Collaborator Author

probably yes but the final say should be when I finish this PR. I don't expect any breaking changes than option normalization (but it should still be 99% compatible for the end user). I'm short on time, preparing for uni exams hence I'm not so active since last week :(

@szmarczak szmarczak mentioned this pull request Dec 8, 2020
4 tasks
Base automatically changed from master to main January 23, 2021 11:37
@sindresorhus sindresorhus added this to the Got v12 milestone Feb 26, 2021
@szmarczak szmarczak mentioned this pull request Mar 14, 2021
71 tasks
@szmarczak
Copy link
Collaborator Author

Closing in favor of #1667

@szmarczak szmarczak closed this Mar 21, 2021
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.

Invert the methodRewriting option logic
2 participants