-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add basic reference docs for createHttpClient
#918
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking this over again at the end and comparing to the createClient
docs above it, I believe there's only one very minor change in here for you, but this may have revealed a couple of bugs in the new docs site.
Creates an HTTP client for interacting with an EdgeDB instance from a browser or edge runtime environment. This function returns an instance of :js:class:`Client` configured to communicate over HTTP using the Fetch API. | ||
|
||
:param options: | ||
This is an optional parameter. When it is not specified the client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an optional parameter. When it is not specified the client | |
This is an optional parameter. When it is not specified, the client |
:param boolean options.tlsSecurity: | ||
Determines whether certificate and hostname verification is enabled. | ||
Valid values are ``'strict'`` (certificate will be fully validated), | ||
``'no_host_verification'`` (certificate will be validated, but | ||
hostname may not match), ``'insecure'`` (certificate not validated, | ||
self-signed certificates will be trusted), or ``'default'`` (acts as | ||
``strict`` by default, or ``no_host_verification`` if ``tlsCAFile`` | ||
is set). | ||
|
||
The above connection options can also be specified by their corresponding | ||
environment variable. If none of ``dsn``, ``credentialsFile``, ``host`` or | ||
``port`` are explicitly specified, the client will connect to your | ||
linked project instance, if it exists. For full details, see the | ||
:ref:`Connection Parameters <ref_reference_connection>` docs. | ||
|
||
|
||
:param number options.timeout: | ||
Connection timeout in milliseconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this got rendered as you intended, but I'm not sure if I'd call this a bug in the docs site, or if we just need to rework the documentation.
We get another "Arguments" heading before options.timeout
. I think we would just want to continue the arguments list after the paragraph about connection options without another heading. I can't think of a case where we would want the current behavior, but I wanted to check in with you and see what you think. I guess if there's an easy fix we can make here we should do that. I'm not sure there is though.
The easy "fix" would be indenting it, maybe as a note inside the description of one of the params, but it's relevant to multiple params so I don't think that really works here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, all of this is copied wholesale from the createClient
docs, so any issues here are also issues there, I'd imagine. Definitely good with taking the time to update these docs both in layout and content though!
will connect to the current EdgeDB Project instance. | ||
|
||
If this parameter is a string it can represent either a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newlines here get collapsed. In the rendered output, "If" is directly against the preceding period with nothing between. I've started a thread about it on Slack. Seems like a bug in the docs.
We probably want to revisit the options docs, but at the very least, this adds documentation for the http client creation function.