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

Changed client creation interface #25

Closed
wants to merge 1 commit into from
Closed

Changed client creation interface #25

wants to merge 1 commit into from

Conversation

martenjacobs
Copy link
Contributor

Replaced dbus.systemBus() and dbus.sessionBus() with dbus.connect({bus: "system"}) and dbus.connect({bus: "session"}), respectively, as discussed in #23

@acrisci
Copy link
Member

acrisci commented May 3, 2019

Actually I'm thinking we can get rid of the convenience function completely to make things even simpler.

I like how the postgres client does it.

const { Client } = require('pg')
const client = new Client()

await client.connect()

Which would look like this for us:

const { MessageBus } = require('dbus-next');
let bus = MessageBus();
// calling connect is optional, but there is a use case to wait for a connection
await bus.connect();

What do you think about that?

Also we should default to the session bus and have an enum for the bus type.

@martenjacobs
Copy link
Contributor Author

// calling connect is optional, but there is a use case to wait for a connection

Do you mean this would cause the first call on the bus to first wait for the connection?

@martenjacobs
Copy link
Contributor Author

let bus = MessageBus();

I suppose we'd put our options in the MessageBus constructor? Could you elaborate how you think this should look?

@acrisci
Copy link
Member

acrisci commented May 3, 2019

Do you mean this would cause the first call on the bus to first wait for the connection?

Yes, this happens anyway though. The connection is complete after the "Hello" method is called on the org.freedesktop.DBus interface to give us a unique name. We buffer any messages before that time and send them in a batch after connection is complete. The bus will connect in the constructor, but awaiting a connection just makes it more explicit. Right now, the bus has a "connect" event for that purpose, but I think awaiting is better.

I suppose we'd put our options in the MessageBus constructor? Could you elaborate how you think this should look?

Yeah something like

let bus = MessageBus({ busType: BUS_TYPE_SYSTEM, authMethods: ['ANONYMOUS'] });

@martenjacobs
Copy link
Contributor Author

Right now, the bus has a "connect" event for that purpose, but I think awaiting is better.

I agree, it's much clearer to the user


Yeah something like

let bus = MessageBus({ busType: BUS_TYPE_SYSTEM, authMethods: ['ANONYMOUS'] });

I do think we need support for custom bus URIs (because I'm using it :) ), so perhaps this would be better:

let localBus = MessageBus({ bus: BUS_SYSTEM, authMethods: ['ANONYMOUS'] });
let remoteBus = MessageBus({ bus: "tcp:host=remote_host,port=1234", authMethods: ['ANONYMOUS'] });

I think busType is kind of a weird name, as the buses (busses?) themselves don't really differ, other than the services connected to them on the other side. I think just using the word bus would be clearer. That way, we could also let the user supply a URI instead of a well-known bus through the same key

* `tcp:host=<HOST>,port=<PORT>`
* `unix:path=<PATH>`
* `unixexec:path=<PATH>,arg1=<ARG>,arg2=<ARG>,argN=<ARG>`
* @param {object} [options.authMethods] - array of authentication methods, which are attempted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to document this because I think I want to change it to pass authenticator classes instead of strings.

@acrisci
Copy link
Member

acrisci commented May 4, 2019

I think busType is kind of a weird name, as the buses (busses?) themselves don't really differ, other than the services connected to them on the other side.

Practically, the difference is how the address is found. There is a different way of finding the address per type and that's what's being expressed by the busType. The busAddress is derived from the type and can be different based on what user session is currently running. You should not use an explicit address in public code because users may want to set this themselves with environment variables.

That way, we could also let the user supply a URI instead of a well-known bus through the same key

In general, the busAddress option is something I don't want people to use at all. I might even remove it until someone comes up with a proper use case for it. So I think it should be a separate key to indicate it's a weird option.

In your case, you should be doing this to indicate the address:

DBUS_SYSTEM_BUS_ADDRESS="tcp:path=address" ./my-script.js

@acrisci
Copy link
Member

acrisci commented Jun 2, 2019

Taking a look at it again, I think this change might be a bit too much of a change for the benefit. Maybe we can revisit it later.

@acrisci acrisci closed this Jun 2, 2019
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.

None yet

2 participants