ocilo / skype-http

Unofficial Skype API for Node.js via HTTP
https://ocilo.github.io/skype-http
MIT License
51 stars 24 forks source link

Expose 3 additional interfaces under the Interfaces namespace #31

Closed mitchcapper closed 7 years ago

mitchcapper commented 7 years ago

Expose 3 additional interfaces under the Interfaces namespace

Do you want these under an "interfaces" namespace or the existing Api namespace. Do we want to move (would be breaking) the interface items under Api to interfaces if you want the new namespace? Let me know will update PR.

demurgos commented 7 years ago

I usually use lowercase names for namespaces, unless I am merging the namespace with an existing class or type (such as Api or Contact). Now, I am not sure if a dedicated namespace for interfaces is really useful. We could maybe export it directly, reuse the Api namespace or provide a namespace with a better name such as events.

Concretely, I'd export ConnectionOptions at the root since it is the same level as the related function connect and the events / resources should be exported directly with import * as events from "..."; import * as resources from "..."; export {events, resources};. This will allow to use it as skypeHttp.events.MessageEvent which feels more natural than skypeHttp.interfaces.MessageEvent.

mitchcapper commented 7 years ago

I went and re-arranged things per the direction. One item you mentioned

should be exported directly with import as events from "..."; import as resources from "..."; export {events, resources}; .

I think you are implying to have users import a specific TS file directly? This leads to some more funky looking code (where you are importing from node_modules and if the file layout changes other issues). I don't mind which way you want to go (as I can code either) but I do think the ability to access usable types from the module import itself is a more clean look.

demurgos commented 7 years ago

I agree that having a main module acting as the single entry point is a very good thing (and I don't like packages that promote deep require such as require("package/file") (no need for ./node_modules/ by the way)). What I am suggesting is to export some sub-namespaces (events, resources) in index.ts so the user don't have to grab the files manually.

Example

index.ts in skype-http

import * as events from "./interfaces/api/events";

export {events};

// This boils down to semantics similar to export * as events from "...", but this syntax is not supported by TS

Consumer code:

import * as skypeHttp from "skype-http";

let event: skypeHttp.events.MessageEvent;
mitchcapper commented 7 years ago

ah, yes I see no downside to this format, do you want to just do this for events and resources or also api (and if also Api do we rename that one to uppercase for export)?

demurgos commented 7 years ago

events is an obvious namespace (errors is another one), but for the rest of the interfaces, they may benefit from a reorganization. For the moment I'd leave them as is and reorganize them in another PR.

Here are some ideas:

mitchcapper commented 7 years ago

ok if I understood you correctly try this.