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

Extracting nodeserver #14

Open
rikkertkoppes opened this issue Feb 23, 2018 · 7 comments
Open

Extracting nodeserver #14

rikkertkoppes opened this issue Feb 23, 2018 · 7 comments

Comments

@rikkertkoppes
Copy link
Contributor

MHub currently already has an exported MClient, which is defined in nodeclient.ts. However, there is no nodeserver equivalent of it.

If one wants to embed a nodejs server into another application, one has to create a hub initialize transports for it, basically a lot of what is defined in mhub-server.ts, which is the cli script.

My proposal is to create a nodeserver.ts, extracting the server relevant bits from mhub-server (like creating the hub, setting up transports and authenticators and adding nodes). This is basically the bottom part of mhub-server.ts

I am happy to work on it and create a pull request for it. Are there any ideas / thoughts you have that I should take into account before working on this?

@rikkertkoppes
Copy link
Contributor Author

rikkertkoppes commented Feb 23, 2018

Progress here: https://github.com/rikkertkoppes/mhub/tree/nodeserver

I am currently cleaning up in small steps. Some intermediate goals are:

  • distinction between normalized config and "loose" config
  • create setup functions for setup parts, make them pure so that they can be easily moved
  • move generic parts to nodeserver.ts
  • throw errors, rather than "die" at lots of places

@poelstra
Copy link
Owner

Cool! Progress looks good so far!

I'll hopefully have some time tomorrow to properly review the changes.

Small remark: please use tabs for indentation (I thought .editorconfig is setup for that, maybe I made a mistake there).

@rikkertkoppes
Copy link
Contributor Author

Tabs, wat?

@rikkertkoppes
Copy link
Contributor Author

rikkertkoppes commented Feb 24, 2018

Can you elaborate on this bit:

const logLevelName = args.loglevel || config.logging;
if (config.logging) {
    // Convert config.logging to a LogLevel
    const found = Object.keys(LogLevel).some((s) => {
        if (s.toLowerCase() === logLevelName) {
            log.logLevel = (<any>LogLevel)[s] as LogLevel;
            return true;
        }
        return false;
    });
    if (!found) {
        die(`Invalid log level '${logLevelName}', expected one of: ${logLevelNames.join(", ")}`);
    }
} else if (config.verbose === undefined || config.verbose) {
    log.logLevel = LogLevel.Debug;
}

(https://github.com/poelstra/mhub/blob/master/src/mhub-server.ts#L160-L175)

So if there is no logging key in the config, then the level always defaults to Debug if verbose and Info (default in Logger) otherwise, even when a cli argument is given.

Seems the first if should check for logLevelName

@poelstra
Copy link
Owner

I think you are right, and it should have been if (logLevelName) indeed.

@rikkertkoppes
Copy link
Contributor Author

I have added loads of commits to the branch, taking baby steps along the way. The main ideas are

  • general effort to make functions pure, that makes them easy to move
  • extracting of config normalizing to an external file, splitting it out in multiple functions
  • effort to not mutate deep objects (e.g. the config), but create new ones instead

Storage and logging remain a bit of a pain, since they are global. Considering you can now create multiple instances of a server, we may want to change that a bit

@poelstra
Copy link
Owner

Storage is now non-global, logging is still global.

Your PR was merged (as it started to contain useful things I'd also like to have on master), so please create a new PR with any follow-up work as necessary. I'll keep this item open to track overall status.

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

No branches or pull requests

2 participants