On Wed, Feb 8, 2012 at 6:07 PM, Isaac Schlueter <[email protected]> wrote:
> I think I agree with pretty much all of this, Mikeal. Exposing ctors
> really invites the wrong kind of composability, but we need to make
> the core systems nicer in the ways you describe. The response header
> handling really needs to be cleaned up. It mostly works, but has some
> odd edges where it doesn't.
>
> Constraints:
> 1. We must maintain backwards compatibility unless it is completely
> unreasonable to do so. This can often be done by re-implementing old
> API surfaces in terms of new ones, but please bear this in mind for
> any change.
> 2. API cleanup should generally not introduce new features, but merely
> make existing features more intuitive, organized, and consistent.
> 3. Performance regressions cannot be tolerated. Express or request or
> fstream can do a bunch of extra junk to make a cuter API, and give up
> some performance in doing so, but node-core must be as fast as
> possible.
>
> Here's a half-thought-out proposal. Please shoot it down in
> constructive ways. :)
>
>
> Expose the actual headers as they will be written. This must be a 2d
> array. An object is not the right data structure, because there can
> be multiple values for the same key, and though it shouldn't, order
> does matter in some very rare edge cases. Like module.exports, if
> this value is overwritten, or manually modified, then that's the
> "real" header.
>
> response.headers = [ [key, value], ... ]
>
If you're going this low level why bother with nested arrays? Why not [
key, value, key, value ... ]? There's no difference and presumably you'll
be using helpers to manipulate anyway.
> Sugar around response.headers.push([key, value]), but giving us a
> place to add intelligent defaults, validation, etc. We should push
> this api everywhere as the default:
>
> response.setHeader("key", "value") // Set a header, adding a second
> one if there's already one by that name.
> response.setHeader("key", "value", true) // Overwrite any existing
> header by that name.
>
Why not separate setHeader and addHeader methods? This should make usage
more obvious. Magic bools like this only serve to confuse.
> Set the status as a property:
> response.status = 200; // should this just default to 200?
>
LGTM. What about the status message text?
> Backwards-compatible shim method:
> response.writeHead = function (status, headers) {
> this.status = status;
> if (Array.isArray(headers)) {
> this.headers = headers;
> } else {
> Object.keys(headers).forEach(function (k) {
> this.setHeader(k, headers[k]);
> });
> }
> };
>
>
> Don't send the actual header on the socket until the first .write() or
> .end(). At that time, response.headerSent = true, and any calls to
> response.setHeader() trigger an error event.
>
What about any direct manipulation of response.headers? Same thing?
>
>
> On Wed, Feb 8, 2012 at 13:39, tjholowaychuk <[email protected]>
> wrote:
> > Yeah, unless you're one of us writing these higher-level things you
> > probably wont even really be concerned about how node does things,
> > since if you're just using straight-up node you're probably happy with
> > writeHead() etc. It's awesome having a low-level core that is not
> > overly opinionated about this sort of thing but at the same time we
> > almost need that, otherwise the community just becomes a diverged
> > mess.
> >
> > It's probably not worth talking about rack-like stuff since that'll
> > never be in core, but I guess the key thing is that we shouldn't need
> > something like that to build pluggable things. Say instead of manually
> > checking accept-encoding all over you want:
> >
> > http.createServer(function(req, res){
> > acceptEncoding(req, res);
> > // go on with regular stuff here
> > });
> >
> > IMHO we should at least supply something reasonable to make that
> > happen. In Connect I'm using a "header" event, it's not as "clean" as
> > something like rack but like I've said before I really dont want to
> > diverge from what core gives us otherwise it renders that stuff
> > useless to other people (like it does now) and splits the community
> > more than it should
> >
> > On Feb 8, 1:35 pm, Mikeal Rogers <[email protected]> wrote:
> >> Yup, none of this is new.
> >>
> >> I think that we need to reach a consensus that this is our goal. I know
> that in core there is a tendency to hold internal state and refrain from
> exposure. I think that some people believe it is "safer" or more "secure"
> and you can see this show a little in the thread about exposing core
> constructors.
> >>
> >> A pre-requisite to me writing patches that solve this is a cultural
> consensus that we are working towards a composable core, and while I know
> TJ is all for it :) I don't think this goal is currently shared across the
> core community and the practice to date seems to lead towards writing
> things initially very restrictive initially and then people like TJ and
> Marco fighting to expose them.
> >>
> >> On Feb 8, 2012, at February 8, 20124:18 PM, tjholowaychuk wrote:
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> > +1 been saying this for years
> >>
> >> > On Feb 8, 1:04 pm, Mikeal Rogers <[email protected]> wrote:
> >> >> All the streams in core are in some half state between declarative
> and composable.
> >>
> >> >> http.ServerResponse is probably the biggest/worst example where:
> >>
> >> >> http.writeHead(status, headers)
> >>
> >> >> can, in some cases, entirely opt out of the composable headers set:
> >>
> >> >> http.setHeader(name, value)
> >>
> >> >> in particular, this happens when you pass an array of tuples as the
> headers argument to writeHead.
> >>
> >> >> Then there is http.ClientRequest, which is not composable at all and
> has no setHeader() method.
> >>
> >> >> I have a similar problem with every stream in core except sockets.
> All of the streams in core take, at some point, a set of parameters that
> set a bunch of internal state and *start* an IO operation. Until that IO
> operation hits a particular event, in the future, none of that internal
> state is actually used. Some of that state can be mutated (is composable),
> some currently isn't (is not composable), and some of it is mutable if you
> access internal properties (ewwww).
> >>
> >> >> If the goal of core is to reach API stability we need to get core to
> a place where it's extensible. There is a similar thread to this effect
> about exposing core constructors, but I'd like to stay on track in this
> thread and just talk about composability because it doesn't have many of
> the same concerns people have about exposing core constructors.
> >>
> >> >> *Most* of the objects in core are accessed by userland as the
> subject of an event. When those objects are not composable we incredibly
> limit the extensibility of core, even when we expose the constructors, and
> it leads to the worst kind of monkey patching. There are even cases where
> core objects that get exposed by libraries have certain methods entirely
> removed because they break composability (I know TJ was considering
> removing writeHead when exposed by Express).
> >>
> >> >> A few things would need to happen to make core more composable.
> >>
> >> >> 1) Internal state needs to become external state.
> >> >> and
> >> >> 2) The removal, or "re-integration", of methods that currently break
> composability.
> >>
> >> >> We may also need to add more events to some objects. Some good
> examples, which have been added, are the 'open' and 'socket' methods on
> fs.ReadStream/fs.WriteStream and http.ServerResponse.
> >>
> >> >> Anywhere we can expose internal state publicly we increase
> composability because we allow new mutations that depend on that state the
> way internal logic depends on it. As we do this we'll also realize more
> places where we break that mutability.
> >>
> >> >> Thoughts?
>
