http-server Remove flatiron/union dependency

We need to get off the outdated flatiron/union dependency because it's no longer necessary because Streams--see https://github.com/flatiron/union/pull/61#issuecomment-443856321 for more.

@thornjad this relates to the PR you sent to flatiron/union. If you've still got coding cycle time, I'd love your help on this one!

PR's from anyone interested are also welcome. 😁

Dec 04 '18 21:12 BigBlueHat

@indexzero @BigBlueHat does union only provide middleware capability and buffering? Or is it more than that?

Dec 20 '18 21:12 thornjad

@indexzero would be the one to answer the union question. Regardless, getting up to something more maintained is important to fix several related issues.

Thanks for all you're doing @thornjad!

Feb 27 '19 19:02 BigBlueHat

@thornjad primarily – union was created when there were no buffered streams natively in Node.js to address that shortcoming. That obviously has not been the case for many years.

In addition to native buffering it's also the main entry point for how the http(s) server is created. That aspect of union has been diligently maintained and improved on over the last several years as create-servers.

With that in mind, we would just need a small "stack" library like connect. Then this bit of code could become:

// 
// ... rest of the existing code above ...
// ... replacing res.emit('next') by invoking a next parameter ...
// 
  if (this.headers) { 
    before.unshift((req, res, next) => {
      Object.keys(this.headers).forEach(header => {
        res.setHeader(header, this.headers[header]);
      });

      next();
    });
  }

  before.push(function onError(err, req, res, next) {
    if (options.logFn) {
      options.logFn(req, res, err);
    }

    res.end();
  });

  const handler = connect();
  before.forEach(layer => handler.use(layer));

  this.serverOptions = { handler };
  if (options.https) this.https = options.https;
}

HttpServer.prototype.listen = function (port, host, callback) {
  const key = this.serverOptions.https ? 'https' : 'http';

  if (!callback) {
    if (typeof host === 'function') {
      callback = host;
      host = null;      
    } else {
      callback = function () {};
    }
  }

  this.serverOptions[key].port = port;
  if (host) this.serverOptions[key].host = host;

  createServers(this.serverOptions, (err, servers) => {
    if (err) return callback(err);
    this.server = servers[key];
  });
};

The above is totally untested, but I did quite a bit of re-reading the code and teasing apart the original the intention of what union does. It should be equivalent with one notable exception: res will have no helper methods (e.g. .json, etc). These are also defined by union.

The only aspect that would be breaking here is if:

Users are consuming http-server programmatically
Users are passing in custom before middleware in their options provided to http-server
Those middlewares expect one or more of these helper methods.

I also did not double check if any of the existing middlewares that http-server uses based on options (e.g. ecstatic, corser, http-proxy) uses any of these helper methods –– but I do not recall ever coming across those in the past.

Hope that helps!

Mar 31 '19 08:03 indexzero

Users are consuming http-server programmatically

Users are passing in custom before middleware in their options provided to http-server

Those middlewares expect one or more of these helper methods.

@indexzero of these 3, I suppose only the last one is the actual concern (i.e. the API surface area changes)?

I have seen several issues/conversations in the past year which point to people doing the first 2 of those...but not sure how dependent they are on using the helper methods.

Apr 02 '19 19:04 BigBlueHat

My thought was that this would certainly be a breaking change. If post-breaking change there are a lot of issues around helper methods being missing then we could consider a small wrapper that adds them.

In the meantime I would read the code for ecstatic, corser and http-proxy to ensure they don't use the helper methods. If they don't give the code I wrote above a shot and lmk how it goes. Happy to review the PR here.

Apr 02 '19 19:04 indexzero

@indexzero thank you for the overview! That sets me on the right path!

As for the "stack" library, we have lots of options. Of course there's connect, but there's also the newish koa and I noticed broadway. @indexzero @BigBlueHat are any of those clear winners? Any other libraries stand out?

Broadway is maintained by @indexzero but hasn't been updated in a few years (though I could maybe put some work into it if there are issues). Connect is well established and I've used it in passing before. Koa is a newer player but is very active and backed by a good team (same people as Express).

This would certainly be a breaking change, and it reminds me to ask, how is http-server's versioning working? My merge train 🚂 is planned as a "major" version but is 0.12. Would we want to pull a Node.js and jump to semver with something like v4.0.0 (or even v13.0.0)? I'm not a fan of that leading 0 that doesn't seem to mean anything.

Apr 19 '19 17:04 thornjad

(for ease of finding issues, list will grow as I find more) Removing union should fix #138, #257, and will significantly affect #388, #487, #135 (stopgap: #479), #474

Apr 22 '19 14:04 thornjad

is there any movement?