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

Better HTTP(S) Request options documentation #1306

Closed
19 of 39 tasks
Giotino opened this issue Jun 5, 2020 · 15 comments · Fixed by #1364
Closed
19 of 39 tasks

Better HTTP(S) Request options documentation #1306

Giotino opened this issue Jun 5, 2020 · 15 comments · Fixed by #1364
Labels
enhancement This change will extend Got features good for beginner This issue is easy to fix ✭ help wanted ✭

Comments

@Giotino
Copy link
Collaborator

Giotino commented Jun 5, 2020

What would you like to discuss?

Since we made some modification to the options, this statement

options
Type: object

Any of the https.request options.

from the documentation is no more valid.

Maybe the individual https.Request options should be documented on the got page.


Node.JS options (checked ones are those already documented by Got)

From http.request

  • agent
  • auth
  • createConnection (not present in README.md)
  • defaultPort
  • family
  • headers
  • host
  • hostname
  • insecureHTTPParser
  • localAddress
  • lookup (not present in README.md)
  • maxHeaderSize
  • method
  • path (might be reviewed)
  • port (not present in README.md)
  • protocol (not present in README.md)
  • setHost
  • socketPath
  • timeout

From tls.createSecureContext (documented https.request)

From tls.createSecureContext (but undocumented in https.request)

@sindresorhus
Copy link
Owner

Yes. It's time to do that. We should take the opportunity to see if any of the https.request options could be improved, like we did with your previous PRs.

@Giotino
Copy link
Collaborator Author

Giotino commented Jun 5, 2020

I think this should be done in 2 steps.
The first is renaming/remapping current https.Request options, giving deprecation errors for the old ones (as we did in the previous PRs).
The second, to be implemented in got 12, is to not use got options with the request function, but crafting a dedicated options object to be used for the request

@sindresorhus
Copy link
Owner

That sounds like a good plan.

@sindresorhus sindresorhus added enhancement This change will extend Got features ✭ help wanted ✭ labels Jun 6, 2020
@Giotino
Copy link
Collaborator Author

Giotino commented Jun 6, 2020

I updated the first message with a list of all the options taken by http.request and https.request

@Giotino Giotino changed the title Documentation problem Better HTTP(S) Request options documentation Jun 6, 2020
@yovanoc
Copy link

yovanoc commented Jun 9, 2020

We can't pass ciphers anymore? ;( I really don't find how to do this now

@Giotino
Copy link
Collaborator Author

Giotino commented Jun 9, 2020

We broke the definition of the https.request options, but they're still there.

So, in got 11, you can do something like this

got('https://example.org', {
  ciphers: '...'
} as any);

The ciphers option is going to be included in the https options, when this happen you'll receive deprecation warnings on ciphers.

@zedd3v

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@markdboyd markdboyd mentioned this issue Jul 17, 2020
4 tasks
@markdboyd
Copy link
Contributor

Issued #1364 to add support for the TLS option pfx to the supported HTTPS options

@szmarczak szmarczak added the good for beginner This issue is easy to fix label Aug 1, 2020
@efalkner

This comment has been minimized.

@Giotino
Copy link
Collaborator Author

Giotino commented Sep 3, 2020

For all the ciphers fans (@yovanoc @zedd3v): I'm going to add the option in this PR #1435

@Giotino Giotino reopened this Sep 13, 2020
@Giotino Giotino mentioned this issue Sep 17, 2020
2 tasks
szmarczak added a commit that referenced this issue Apr 26, 2021
Related with #1306
TODO: tests
@szmarczak
Copy link
Collaborator

Enabled more HTTPS options here: 83575d5

@Giotino We just need to do the tests and add docs.

@Giotino
Copy link
Collaborator Author

Giotino commented Apr 27, 2021

Enabled more HTTPS options here: 83575d5

@Giotino We just need to do the tests and add docs.

https://github.com/Giotino/got/tree/https-tests

I adapted the tests from #1435.
The tests cover:

  • ciphers
  • honorCipherOrder
  • minVersion

Other options are a little bit tricky to test.

@szmarczak
Copy link
Collaborator

Great! Can you submit a PR?

@szmarczak
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features good for beginner This issue is easy to fix ✭ help wanted ✭
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants