-
Notifications
You must be signed in to change notification settings - Fork 64
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
Implement named instances support #63
Conversation
Also deprecate `admin: true` and `edgedbadmin:` protocol, as they aren't needed if proper permissions are set up. And was introduced mainly for command-line tools.
7d993be
to
d9cb1a6
Compare
This adds positional argument to `connect()` and `createPool()` functions which can be either DSN or an instance name.
d9cb1a6
to
671f43b
Compare
@@ -9,19 +9,21 @@ API | |||
Connection | |||
========== | |||
|
|||
.. js:function:: connect(options) | |||
.. js:function:: connect(dsn, options) |
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.
Paul, I don't like breaking the API just yet. I'd do this differently: let's make options
to be of type ConnectConfig | string | null
.
We can then gradually introduce "instances" - they'll be a separate JS type that we will be able to check with an instanceof
- and ban plain objects (currently ConnectConfig
)
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.
Well, then you can't set timeout. Or the API would look like:
connect(name_or_dsn_or_options: string | ConnectOptions | null, options: ConnectOptions | null)
(and then throw error if both are objects?) I.e. jQuery-age libraries had a lot of API looking like this, but in 2020...
And then what to do with createPool
? This function without options is next to useless.
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.
As discussed on the call, let's try:
Support all connect(instanceName)
, connect(ConnectConfig)
and connect(instanceName, ConnectConfig)
use cases. The signature will likely look like this:
connect(ConnectConfig | string | null, ConnectConfig | null)
The code will need to validate that only one ConnectConfig
is actually passed.
We'll start issuing deprecation warnings once we fully agree on the new instance API and start implementing it.
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'd like to approach this different for now, essentially without breaking backwards compat (see my only comment for this review). We'll have an opportunity to deprecate/change the API later.
Updated the code. Docs insist on the new signature and mention compatible form as a side note. |
// or | ||
await connect({dsn: 'edgedb://localhost'}) | ||
|
||
But this form is deprecated and will be removed in the future. |
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 adds positional argument to
connect()
andcreatePool()
functions which can be either DSN or an instance name.This is a part of what's discussed in edgedb/edgedb#1683 other things will follow in the next release.
This also deprecates
admin=True
parameter andedgedbadmin
schema, asthey aren't needed if proper permissions are set up. And were introduced
mainly for command-line tools.
Supersedes #62