Restructure API and module

fbbdev commented 8 years ago

I would restructure the module around the concept of platform: where a platform (or any other name you like better) is a possible deployment environment represented by an object with two methods and a name property:

{
    name: 'someplatform',

    detect: function () {
        if (/* correct environment detected */)
            return true;
        else
            return false;
    },

    serve: function (app) {
        // ...
    }
}

The API

The module API should be composed of two main functions: detect and serve. The former detects the current environment and returns a platform object; the latter calls detect, then delegates to the serve method of the platform object.

function serve(app) {
    return detect().serve(app);
}

The serve function should also be assigned to module.exports so one can require the module and call the returned object, just like it is now.

The module should export an object named platforms (or the like) containing all available platform objects.

var ad = require('http-autodetect');

if (ad.platforms.fastcgi.detect())
    ad.platforms.fastcgi.serve(app);

There should be a register or use method to allow loading extensions:

// autodetect-myserver
module.exports = {
    name: 'myserver',
    detect: function () { /* ... */ },
    serve: function (app) { /* ... */ }
};

// app.js
var ad = require('http-autodetect'),
    myserver = require('autodetect-myserver');

ad.use(myserver);
// ...
ad(app);

The name property of platform objects is used to avoid duplicates.

Finally, a choice should be made about currently available platforms:

keep all implementations in this module
move all platforms to separate modules, keeping only http as a fallback, and leave choice to the user
move platforms to separate modules, but add the most common ones (FastCGI, CGI, Passenger maybe?) as dependencies and load them automatically by default. Http is still kept as fallback
detect method implementation

The detect method should iterate over available platforms and call their detect function to select the correct one. This approach however creates a problem: which is the correct testing order? What if two or more platforms give a positive result?

I think a good solution - and the one I would adopt - would be to call detect on all platforms and throw an exception if more than one signals detection. The http platform's detect method should always return false, and http is only used as fallback if no other platform is detected.

function detect() {
    return Object.keys(platforms).reduce(function (detected, key) {
        var platform = platforms[key];
        if (platform.detect()) {
            if (detected !== platforms.http)
                throw /* exception: more than one platform detected */;
            return platform;
        } else {
            return detected;
        }
    }, platforms.http);
}

fbbdev commented 8 years ago

A good effect of this change is that people could directly maintain platform modules for their own solutions. For example, I could maintain an autodetect-fastcgi module and keep it up-to-date with future developments in node-fastcgi without having to write you every time I push a new version. Or you could create a github organization to host extension modules and give commit access for each module to individual maintainers.

binki commented 7 years ago

This makes a lot of sense and should be done. Some parts are concrete enough to start working on now, even, but I would like to deliberate on some aspects:

Platform Overlap Resolution

A generalization of your resolution for when multiple platforms match would be to let platforms be weighted. The default weight could be 0, platforms could specify lower weights to be preferred and higher ones to be ordered later/as fallbacks. The platforms would be grouped by weight. The search would go group by group. The first group with any matches would throw if that group contains multiple matches, succeed if that group contains one match (and stop the search there), or continue on to the next group if there are no matches in that group. It’s just a small extension to what you suggest and would even allow you to, e.g., prefer your custom platforms over stock ones.

Dependency Injection

It would be nice to allow injection of custom platforms to have an official well-supported way to specify additional platforms without needing the code to be involved. I.e., instead of needing to call autoserve.register(myCustomPlatform), if serve() could look for some deployment-generated dotfile and automatically load platforms listed there. Well, after thinking this through, I think I’ve realized that this isn’t the way to go because you could instead have your hosting environment/deployment script just generate a module on the fly which mounts your app in an autoserve environment. Actually, maybe this requires a “nested autoserve” platform?

Say you have a deployment script to support a ridiculous hosting environment that requires the custom-ridiculous-autoserve-platform. You want to deploy autoserve-compatible app app. The deployment script could generate the following script:

#!/usr/bin/env node
const autoserve = require('autoserve');
autoserve.register(require('custom-ridiculous-autoserve-platform'));
const app = require('app');
// Grr, I can’t figure out how to reconcile things that this point.
// Depending on what require('app') returns, or maybe we need
// to require the user to pass `module` and detect if we’re the
// main script or just being required() and the autoserve-nesting
// platform would just write to `modules.exports` in that case?
autoserve(module, app);

Actually, I got stuck. There are two issues. require('app') would run something which calls autoserve(app); (or would/should it?), which means it would do its own platform discovery and not export something mountable. The second issue is that even if require('app') did return something mountable, how would I indicate to the mountable thing the right version of autoserve.getBaseUrl()? I know that if I could assume things were Express apps (which supports its own baseUrl tracking when mounting with app.use('/path', subApp)) I could do the following:

#!/usr/bin/env node
const expressAutoserve = require('express-autoserve');
expressAutoserve.register(require('custom-ridiculous-autoserve-platform'));
expressAutoserve(require('app'));

So currently the only thing I can think of is this sort of solution, and I’m back to using deployment-script-generated dotfiles:

Deployment script runs npm install app in some directory then moves node_modules/app to the deployment directory.
Deployment script runs npm install in the deployment directory.
Deployment script runs npm install custom-ridiculous-autoserve-platform in the deployment directory.
Deployment script writes ["custom-ridiculous-autoserve-platform"] to a file named .autoserve-deploy-platforms in the deployment directory.
Deployment script does platform-specific things (e.g., emitting .htaccess or calling something to register the deployment with ths hosting system).

Then, if autoserve() is modified to look for the dotfile and automatically registers all listed platforms in there, a generic app which merely calls autoserve(handler); can be deployed to any hosting environment, assuming the platform module supports all of the used features.

If you understand the deployment-time dependency injection issue I describe and have any opinions, please comment ;-).

fbbdev commented 7 years ago

Weighting platforms makes sense to me, of course with a default. Maybe all platforms should have 0 as default (except maybe the http fallback) and we should have an API to change the weight, something like autoserve.[weight|prefer|sort](platform_id, weight). This in turn could be very useful for dependency injection, as I'm going to detail below.

Dependency injection

If I understand correctly, you want a way to inject additional platforms without changing the main application file. The wrapper script approach is going to work. Modules are loaded only once and garbage-collected, otherwise global variables would not work correctly.

Let's say we have a typical application file that imports autoserve and starts serving. All you need to do is to register some platforms before that file is run by the interpreter. Thus this code:

#!/usr/bin/env node
const autoserve = require('autoserve');
autoserve.register(require('custom-ridiculous-autoserve-platform'));
require('app');

will be just enough.

This is where configurable weights get really useful, because one could keep the defaults in the application file and change them in the deployment script.

binki commented 7 years ago

Ah, I see. That should work. Thanks very much for the guidance!

So, to reiterate the proposed with the above discussion:

{
    name: 'platform',

    detect: function () {
        if (supportedEnvironment) {
            return true;
        } else {
            return false;
        }
    },

    serve: function (app) {
        // …
    },

    // Lower means platform is considered before others.
    // If multiple platforms with same weight have detect()
    // return true, that is an error. Leave unspecified to default
    // to 0. This property should be writeable so deployment
    // scripts can alter the default search order.
    weight: 0,

    getBaseUrl: function (req) {
        // req is an object that serve() passed to app(req, res).
        // The platform must produce the baseUrl. Leave unspecified
        // to use default implementation which returns '/'.
    }
}

The object exported by the module will be like:

const autoserve = function () {
    // implementation that calls autoserve.detect().serve().
};
Object.assign(autoserve, {
    serve: autoserve,
    detect: function () {
        // Returns the detected platform object according to weight rules.
    },
    platforms: {
        // built-in node http module implementation
        http: {},
    },
    // I think I like the name register because it’s unlike express’s/connect’s use().
    register: function (platform) {
        autoserve.platforms[platform.name] = platform;
    },
});

Some core platforms will be included right in the autoserve module including ones that either require almost no implementation or do not require external modules or that are adequately popular (I guess node-fastcgi ;-) ). But the option to register() and the possibility to support being deployed to something arbitrary would be documented and people can publish other modules as they see fit.

I’ll go and do this.

fbbdev commented 7 years ago

Ok, the only thing missing in my opinion is an API entry to set platform weights - you don't want random people changing properties of platform objects, do you? :wink:

About core platforms, I would move everything except http to separate modules and make autoserve depend on them and register them by default. This is for philosophical reasons (do one thing and do it well) and also because I hope to become a maintainer of the would-be autoserve-fastcgi module. :grin:

Also I hope I can propose a last minute change (please don't hate me :cold_sweat:). The current implementation of the http platform looks for a property named port on the app object or for the PORT environment variable. I think it would be better not to impose such behavior on the user: you could extend the autoserve function to have the signature autoserve(app [, options]), where the optional second argument should be an object containing platform names as keys and option objects as values (e.g. { http: { port: 80 } }). The autoserve function would then become:

function autoserve(app, options) {
    options = options || {};
    const platform = autoserve.detect();
    platform.serve(app, options[platform.name]);
}

Of course each platform would define sane defaults, e.g. http would host by default on port 3000. The serve method on interested platforms should also accept an additional argument for the options object.

fbbdev commented 7 years ago

I forgot one last thing: lower weight should mean platform is considered after, not before others. The most common action in a deployment script would certainly be to prioritize some platform over others, and this should not require using negative numbers.

binki commented 7 years ago

Ok, the only thing missing in my opinion is an API entry to set platform weights - you don't want random people changing properties of platform objects, do you? :wink:

My thought on that was that platform modules could leave their properties mutable but the registration would make an immutable copy of the PODs. So to change a pre-registered platform’s priority, you would have to make a copy with your specified weight and then just reregister it. E.g.:

const autoserve = require('autoserve');
// Force the 'http' platform for fun.
autoserve.register(Object.assign({}, autoserve.platforms.http, {
    weight: -1000,
});

Because registrations are keyed by name, this would remove the existing 'http' platform and replace it with the overridden one. But setting the version stored in the registry as an immutable shallow copy of the platform would allow the register() function to perform some amount of validation and avoid having the platform be modified after it passes validation—to a point. The hope is that this allows the full required flexibility while keeping you from shooting yourself in the foot as a user.

I would move everything except http to separate modules and make autoserve depend on them and register them by default.

Sounds good.

The current implementation of the http platform looks for a property named port on the app object or for the PORT environment variable.

The reason I did that was because I thought it seemed to be a generally held convention. But I can’t actually find where I got the get('port') code from. So I’ll need to remove that and add the optional options argument in a future commit.

I forgot one last thing: lower weight should mean platform is considered after, not before others.

When I think of “weights”/ordering, I think of it as just an ascending rank describing the order in which to search for a match. It seems like it’s arbitrary which way you choose to go. I’ll just make my decision clear in the documentation, or if you’re convicted of this (find any node framework precedents?) I can rename it from weight to priority and invert the order ;-).

Please keep the comments coming, hopefully I’ll commit a bit of progress soon :-).

binki commented 7 years ago

you could extend the autoserve function to have the signature autoserve(app [, options])

Actually, with my (imaginary, I guess xD) deployment script use case, I don’t think this would work quite right. If the app itself calls autoserve(app, options), how does the deployment script get any say? I think instead that each platform should have a public mutable options property which it can read. The register() function would generate it. With that, you could host an application with platform-specific options like:

const autoserve = require('autoserve');
autoserve.register(require('custom-autoserve-platform'), {
    // Could specify options here when registering for convenience.
});
// Or mutate them after the fact:
autoserve.platforms['custom-autoserve-platform'].options.debug = true;
// And then launch the wrapped autoserve application.
require('app');

Then the implementation of platform.serve() can access these in this.options and handle them however it wants.

Thoughts?

fbbdev commented 7 years ago

My thought on that was that platform modules could leave their properties mutable but the registration would make an immutable copy of the PODs. So to change a pre-registered platform’s priority, you would have to make a copy with your specified weight and then just reregister it.

That's not a bad idea, yet I advocate the addition of an API to automate that copy-modify-register process. Three lines of code and all that Object.assign cruft are too much just to change a weight, and they don't express intent well. You have two problems here: API ergonomics (for the developer) and readability (for the maintainer).

I can rename it from weight to priority and invert the order

When I think of weight or priority, I think of the same concept (higher weight/priority comes first, like in "high priority"); when lower values come first, I would call that order/ordering. It's just a naming issue, but my point was - again - more about ergonomics: using negative numbers to prioritize things feels strange to me. The only precedent I can think of is process priority on linux (where indices range from -20 to 19 with -20 being the highest priority) and again this feels very weird. But maybe it's just me. :smile:

Actually, with my (imaginary, I guess xD) deployment script use case, I don’t think this would work quite right.

You're right. But I'm worried that the apps would end up reregistering platforms just to change options, thus overriding settings made by the deployment script, including weight. Maybe you could support both the argument on the autoserve function and an override method for dependency injection?

fbbdev commented 7 years ago

Thinking about it, one could add an API entry like this:

autoserve.options = function(platform, options) {
    var originalServe = platforms[platform].serve;
    autoserve.register(Object.assign({}, platforms[platform], {
        serve: function (app, opts) {
            return originalServe.call(this, app, Object.assign({}, opts, options));
        }
    }));
}

This would allow overriding options passed to the autoserve function call without needing additional properties on the platform object.

Then you would just specify in the documentation that this method is meant for dependency injection.

fbbdev commented 7 years ago

Or maybe you could just add a single method, called override or the like, which only automates the copy-modify-register part. This would allow to change both options and weight, but it would put some more burden on deployment script authors.

autoserve.override = function(platform, obj) {
    autoserve.register(Object.assign({}, platforms[platform], obj));
}

// Usage in deployment script:
autoserve.override('fastcgi', { weight: -10 });
var originalServe = autoserve.platforms.http.serve;
autoserve.override('http', { serve: function(app, opts) {
    return originalServe.call(this, app, Object.assign({}, opts, { port: 2000 }));
});

I don't really know which way would be better. Anyway, I'm totally against using platform object properties to set options because this puts on platform module developers the burden of getting the logic right.

fbbdev commented 7 years ago

Sorry for posting so many comments. Maybe the option logic could stay entirely out of platform objects: you could have a global property of the module, named options, which would override the option argument passed to autoserve:

module.exports = function (app, options) {
    options = Object.assign({}, options || {}, module.exports.options);
    var platform = module.exports.detect();
    platform.serve(app, options[platform.name]);
}

module.exports.options = {};

And in the deployment script you would write autoserve.options.http = { port: 2345 }

binki commented 7 years ago

Making options a module property sounds like a proper way to go. Then autoserve will make sure the platform’s specific options are passed to platform.serve(app, options). That is much more obvious/maintainable than requiring the platform code to look for this.options.

With your request to reverse weight, now I’m more confounded on what to do :-p. I still for some reason thing that if it’s called weight it should go in the order it is right now. I guess I’ll rename it to priority and then reverse it.

Regarding the override() idea, I guess providing override(platformName, obj) as convenience would be helpful. But it seems like I shouldn’t have to do that. In C# I’d implement that as an extension method generalized to IRegistry (I don’t like JavaScript’s alternative of prototype injection, it’s totally different). But I should just stop complaining and do it ;-).

binki / autoserve