Remove flatiron/union dependency

[BigBlueHat]

We need to get off the outdated flatiron/union dependency because it's no longer necessary because Streams--see flatiron/union#61 (comment) 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. 😁

[thornjad]

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

[BigBlueHat]

@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!

[indexzero]

@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!

[BigBlueHat]

• 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.

[indexzero]

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.

[thornjad]

@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.

[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

[gnganapath]

downgrade you node js from 12 to node 10. It helps to run angular micro front ends

[ericprud]

Hi @thornjad , some guy named @thornjad suggested you add #537 to your dependency list.

Got an ETA on this? Also, any idea if the next node will remove the _headers interface?

[github-actions]

This issue has been inactive for 180 days

[aytvill]

still there in July 2024