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

Got 12 improvements #1667

Merged
merged 187 commits into from
Apr 11, 2021
Merged

Got 12 improvements #1667

merged 187 commits into from
Apr 11, 2021

Conversation

szmarczak
Copy link
Collaborator

@szmarczak szmarczak commented Mar 14, 2021

This is a redo of #1521

Fixes #1668 (tested)
Fixes #1663 (tested)
Fixes #1538 (tested)

If this gets merged, we need to open an issue to fix the docs. I'll do them, as well as migration notes etc.

**Note**: By using the `Options` class we introduce a regression! In some cases where option normalization throws `error.options` is `undefined`. ~~This could be solved via **not throwing in the `Options` constructor**, but setting a property e.g. `lastKnownError` instead. That way `error.options` would be defined. It's just a matter what we do consider more friendly.~~ Ok found a better way - let's define `error.options` on the error thrown by the `Options` constructor. Later!

  • Improved option normalization (new class: Options)
    • new Options(url | options, options?, defaults?)
      Note: Only the first url instance is used.
      Meaning: got('https://a', {url: 'https://b'}) will actually make a request to a.
      If you expect to replace the url option, then you need pass the initial url like this:
      got({url: 'https://a', ...otherOptions})
    • options.merge(newOptions) replaced got.mergeOptions & Request.normalizeArguments
      Note: It does NOT merge the url option - but it does merge the searchParameters option
    • Throws when an option does not exist
    • Dropped legacy URL support, use options.url instead (a URL instance)
    • Creates a brand new object for the native http options
    • No implicit timeout declaration, use {timeout: {request: 1000}} instead
    • No implicit retry declaration, use {retry: {limit: 3}} instead
    • dnsLookupIpVersion is now a number (4 or 6) or undefined
  • Switched to get-stream (getting a buffer from a stream)
  • Refactored core/index.ts
  • Refactored as-promise/index.ts
    • Moved the onCancel handlers out of makeRequest in order to prevent them being called multiple times (.destroy() needs to be called only once)
    • The Request class now provides rawBody if request._noPipe is true (used internally by Promise API)
    • Switched from for/entries to classic for loop when iterating over afterResponse hooks in order to improve performance
      Why: .entries() sends a query to get enumerable properties + generates an array - but we don't need that since we already know the enumerable properties since it's an array and we don't need to generate a new one
    • Fixed Request options not being reused on retry (needs a test tested)
  • Moved Response-related stuff to core/response.ts
  • Moved error definitions to core/errors.ts - you now have to import them directly, meaning you cannot access them via got.RequestError
  • New error type: RetryError which always triggers a new retry when thrown
  • Moved waitForOpenFile to core/utils/wait-for-open-file.ts
  • Added missing once types for Stream API
  • Switched from symbols to privates
  • redirectUrls and requestUrl now give URL instances
  • The retry timeout in request._beforeError is now promisified
  • Renamed request.aborted to request.isAborted
  • Renamed the lookup option to dnsLookup
  • The beforeRetry hook now accepts only one argument: error
    To retrieve:
    - the underlying request: error.request
    - the underlying options: error.options
    - current attempt number: error.request.retryCount + 1
  • The beforeRedirect hook's first argument (options) is now a cloned instance of the Request options.
    This was done to make retrieving the original options possible. To retrieve the original options, use plainResponse.request.options.
  • The redirect event now takes two arguments: updatedOptions and plainResponse
  • The retry event now takes only one argument: error (reverted; reason: the current API is much nicer)
    To get the attempt count, use error.request.retryCount + 1
  • The socketPath option has been removed. Use the unix: protocol instead.
  • The retryWithMergedOptions function no longer returns a Promise. Keep in mind that this should be the last function being executed in an afterResponse hook.
  • error.options is now enumerable
  • defaults.handlers don't need a default handler ((options, next) => next(options)) now.
  • Renamed options.https to options.httpsOptions
  • Removed internal _isAboutToError function
  • Renamed the searchParams option to searchParameters (reverted)
  • You can no longer set options.agent to false. To do so, you need to define all the options.agent properties: http, https and http2.
  • Refactored Got interface (instances, defaults, etc.)
  • The init hook now accepts a second argument: self, which points to an Options instance
  • The init hooks are ran only when passing an options object explicitly. It's no longer called on got('https://example.com') - in order to trigger init hooks, you need to call got('https://example.com', {})
  • When passing a url option when paginating, it now needs to be an absolute URL - the prefixUrl option is always reset from now on. The same when retrying in an afterResponse hook.
  • Fixed tests
    • Fix test/arguments.ts
    • Fix test/create.ts
    • Fix test/normalize-arguments.ts
    • Fix test/hooks.ts
  • got.extend(...) will throw when passing some options that don't accept undefined - undefined no longer retains the old value, as setting undefined explicitly may reset the option
  • In order to import Got in CommonJS now you have to do const {got} = require('got')
  • Created a new documentation

To be fixed

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 szmarczak changed the title Improve options normalization Improve options normalization (updated) Mar 15, 2021
@szmarczak
Copy link
Collaborator Author

szmarczak commented Mar 15, 2021

So I've benchmarked option normalization... and it's great <3 130k/s when normalizing URL. But when just cloning the options... 1M/s. That means we're bound by native JavaScript performance. Blazing fast.

Update: So we're down to 400-500k/s. But that's still better than the current 90-100k.

@szmarczak

This comment has been minimized.

@szmarczak szmarczak marked this pull request as ready for review April 9, 2021 18:07
@szmarczak

This comment has been minimized.

@szmarczak
Copy link
Collaborator Author

Oh, and one major thing: this PR uses TS default export.

@sindresorhus
Copy link
Owner

Fixes #1668 (needs a test)
Fixes #1663 (needs a test)
Fixes #1538 (needs a test)

Did tests get added for these?

@szmarczak
Copy link
Collaborator Author

Not yet, I can get them added in an hour. Currently I'm finishing my PR in ava/typescript.

@szmarczak
Copy link
Collaborator Author

Added the tests :)

@sindresorhus
Copy link
Owner

Created a new documentation

Can you open an issue for this?

@sindresorhus
Copy link
Owner

This is looking really good! Super nice work as always. 🙌

@sindresorhus
Copy link
Owner

Renamed the searchParams option to searchParameters

I don't think we should do this though. Can you revert that? While I generally prefer full words, I think it's more important here to use the same name as Fetch, and also, it's not worth doing a breaking change just for that.

@sindresorhus sindresorhus merged commit 3c23eea into main Apr 11, 2021
@sindresorhus sindresorhus deleted the options-type branch April 11, 2021 05:20
@szmarczak
Copy link
Collaborator Author

Sure, will do this in a few hours.

@szmarczak szmarczak mentioned this pull request Apr 11, 2021
13 tasks
@szmarczak
Copy link
Collaborator Author

7b3cbfd

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