Advertise Browser Support

[wesleytodd]

I am interested to know if you all think it is worth mentioning that this module is compatible with browsers. I threw together an example of using this module in front-end code, and it works perfectly fine.

It is a big deal to say that the module is "Browser Compatible" because of the support issues, but the current implementation works in Chrome & FF (which is all I tested). And after reading most of the code I don't see anything that will clearly break it in other browsers (at least with the required polyfills).

[dougwilson]

We could do it if someone is willing to help me setup automated browser testing (because otherwise it's just too easy to accidentally break browser support). I just have never setup browser testing before.

[wesleytodd]

I believe we had something setup internally for this, probably with Sauce Labs, I will ask our QA lead if he has any suggestions and post back.

Really I am just glad to hear that you are interested in this being a "supported feature" of the module. I am going to write a push-state router implementation wrapping this module, and didn't want to base my stuff on something that might not be supported in the long run.

[dougwilson]

No problem. There are modules that are definitively Node.js-specific, and others (like this) that are basically neutral (that don't intend to not work in browsers, but it's not the author's primary concern). If I can set it up so I get automated browser tests, then I'll know when a change breaks and can just make it not break, thus providing support :)

[rlidwka]

I'm curious, what it can be used for in browser environment? Some kind of a single-page app?

[wesleytodd]

@rlidwka Yeah, we are doing an isomorphic app. Basically I am making a routes.js file that looks like this:

var index = require('./handlers/index'),
    foo = require('./handlers/foo');

module.exports = function(router) {
    router.get('/', index);
    router.get('/foo/:bar', foo);
};

Where router is an express app on the server and a wrapper around this router on the client. Then I make each middleware a module and use browserify's browser field in the package.json to load the client or server side version of the middleware.

We are using React for rendering and a wrapper around XHR/Request or data loading. It enables almost all of our modules to work on both the server and the client.

[wesleytodd]

For backstory:

Our last application was an Angular app with a Go backend. One of the most common bugs we had was weird behavior because we forgot to define all the proper routes in both Go and javascript. This was both a consequence of our architecture (multiple angular apps on a single domain) and having the routes duplicated. So this time around we really want to avoid the duplicated route issue.

[wesleytodd]

Ok, so going more deeply into the browser testing I found that __proto__ is not supported before IE11. This small change fixes it:

// Was:
router.__proto__ = this

// Now:
for (var i in this) {
  router[i] = this[i];
}

Also, it looks like you could get IE9 support with two polyfills, Function.bind and Array.isArray. bind is only used once and `isArray is used twice (not counting dependencies). So they might also easily be replaced with older style implementations.

Anyway, I can look at putting together a PR for that if the change seem fine to you all.

As for the automation, Sauce Labs has free accounts for OS projects, so if you, @dougwilson, want to set one up I could also do a PR for that integration.

[dougwilson]

Anyway, I can look at putting together a PR for that if the change seem fine to you all.

I would rather find a Object.setPrototypeOf polyfill to use, rather that bloating the memory for Node.js users from all the property copying. It would also break people who add stuff to Router.prototype after they constructed router instances.

Sauce Labs has free accounts for OS projects, so if you, @dougwilson, want to set one up I could also do a PR for that integration

Sounds good to me.

[dougwilson]

Anyway, I can look at putting together a PR for that if the change seem fine to you all.

I would rather find a Object.setPrototypeOf polyfill to use, rather that bloating the memory for Node.js users from all the property copying. It would also break people who add stuff to Router.prototype after they constructed router instances.

To expand on this, what I mean is we can just replace that with a call to Object.setPrototypeOf and the browser polyfill can decide to just do a property copy, if it desires, but the Node.js people can still have their __proto__ goodness.

[wesleytodd]

Yeah, great point. I was unsure of that because of the exact thing you pointed out. I will look at doing that first thing tomorrow morning.

Also, if you are interested, here is the progress I made today. I still have quite a bit of work to do for the popstate handler and coming up with a good req/res abstraction instead of my plain objects. But it functions as it should.

So thanks for making my dreams come true! I really always wanted to be able to do isomorphic apps, this is the first time I am actually able to make it happen.

[wesleytodd]

Does anyone happen to know of a good polyfill for Object.setPrototypeOf? I have looked for a stand alone version that would support back to IE8 and could not find one. The best looking one I found is in core-js. But it is heavily integrated into that ecosystem. If anyone has any better suggestions than trying to pull that out into a standalone version let me know.

[dougwilson]

Polyfill can be the following for our purposes:

Object.setPrototypeOf = { __proto__: [] } instanceof Array
  ? function setPrototypeOf(obj, prototype) { obj.__proto__ = prototype }
  : function mixinProperties(obj, prototype) { for (var prop in prototype) { obj[prop] = prototype[prop] } }

[dougwilson]

Apparently __proto__ is part of the ES6 specification.

[wesleytodd]

MDN certainly has some conflicting information on this subject. Last thing to consider here is avoiding the pollution of the global namespace. I threw together a little npm module to avoid that polution, if it looks good to you I will put together a PR using this module.

[dougwilson]

Link to module :)?

[wesleytodd]

HAHA, yeah that would help! https://github.com/wesleytodd/setprototypeof

[nateps]

Hey! I'm planning on using this module for the new recommended routing library for DerbyJS (http://derbyjs.com/). Everything else in the framework (including the current router based on older Express code) supports IE 9+. @wesleytodd's recommendation for falling back to property mixins looks like the best solution given the current API. Could we get it merged in?

Looks like .bind is only used in one specific case. Could make the call support the specific arguments only. Using a simple function closure is faster than calling bind and apply anyway.

var defer = typeof setImmediate === 'function'
  ? setImmediate
  : function(fn){ process.nextTick(fn.bind.apply(fn, arguments)) }

could simply be:

var defer = typeof setImmediate === 'function'
  ? setImmediate
  : function(done, layerError) {
      process.nextTick(function() {
        done(layerError)
      })
    }

Agree?

[dougwilson]

Looks like .bind is only used in one specific case. Could make the call support the specific arguments only. Using a simple function closure is faster than calling bind and apply anyway. Agree?

I don't agree; the setImmediate polyfill needs to support n-arguments, otherwise it's not a polyfill at all and it'll randomly break because we're not testing in an environment that uses it. Please suggest a different fallback that support n-arguments :)

As far as this whole thing, I can merge in, but not until I can actually test it (i.e. I need tests that fail if the changes are missing). Without testing, nothing is guaranteed and everything will just randomly break in the future. If someone can provide god instructions on how I can setup a browser CI for this project, that's all I'm waiting for.

[nateps]

It isn't really a polyfill in the current implementation, since it is a function that called defer. Could use a more specific name like deferDone to make it more clear that it is single purpose. I figure might as well make it simple given that it is the only use anywhere in this module.

Could of course write a true polyfill pretty easily, but it would be more code and slower.

[dougwilson]

That's irrelevent; it's a simple setImmediate polyfill; it's simply called defer so I don't have to overwrite the setImmediate name, otherwise I would have just called it setImmediate.

[wesleytodd]

Whatever the solutions we end on are, the real problem is that it is not supported, officially supported, until we have automated tests. Do you have a good setup in derby? If so can you setup something similar here?

[nateps]

We set something up with testling, but that has broken and haven't heard anything about James fixing it. Will probably switch to Sauce Labs, which is the only other good option of which I'm aware. For now, we've been manually running the tests in VMs. Microsoft makes it free and relatively easy to download different versions of Windows for testing at least: https://www.modern.ie/en-us/virtualization-tools#downloads

Re: setImmediate, you could make it generic:

var defer = typeof setImmediate === 'function'
  ? setImmediate
  : function(fn) {
      process.nextTick(function() {
        fn.apply(undefined, arguments)
      })
    }

Or https://github.com/YuzuJS/setImmediate if you want to actually polyfill it.

Alternatively, how about removing this altogether, since it is only needed in Node <= 0.8, which is pretty old at this point. The Express version of this is calling setImmediately directly at the moment: https://github.com/strongloop/express/blob/master/lib/router/index.js#L188 If people want to polyfill it, they can always include the module above and polyfill it themselves.

[dougwilson]

Alternatively, how about removing this altogether, since it is only needed in Node <= 0.8

Um, telling me to drop support for Node.js 0.8 is the same as me saying I won't support IE < 9. You're not really starting off great asking me to support old browsers and then saying I shouldn't support old Node.js versions...

[dougwilson]

If people want to polyfill it, they can always include the module above and polyfill it themselves.

Sure, then people can include .bind polyfills themselves as well. Your arguments seem contradictorily to me...

[blakeembrey]

Just commenting to say the generic code looks more like:

var defer = typeof setImmediate === 'function'
  ? setImmediate
  : function(fn) {
      var args = Array.prototype.slice.call(arguments, 1)

      process.nextTick(function() {
        fn.apply(null, args)
      })
    }

But since it's only used in a single place, I don't see why it couldn't be a function that just accepts two arguments? It's not a proper polyfill, but we don't need one for the only use of it.

[dougwilson]

Regardless of what is here: my offer is still out. I just need to figure out (or someone can help me, which would get it done way faster) testing modules on web browsers in a CI and then I'll pick away and casting the widest browser support I can. I just need help setting up web testing on a CI.

[mikermcneil]

@wesleytodd re req/res abstraction, we're mocking these in sails for the purpose of socket.io requests. That code is definitely not browser compatible but might be useful if you need a list of properties on req/res you need to mock

[wesleytodd]

https://github.com/wesleytodd/nighthawk/blob/master/lib/request.js

That is what im doing right now. But link up yours so I can see what I am mssing!

[wesleytodd]

Actually, maybe we can see about abstracting it out so we have something to share. I can take a look at that this weekend if you think its possiable.

[nateps]

Separate from the issue of old browser support, my understanding is that .bind() is not as performant as a simple closure, and it is easy to avoid using it. This of course is the kind of thing that micro benchmarks are going to exaggerate, but I've seen this mentioned repeatedly (https://jsperf.com/bind-vs-self-closure for example).

Personally, we just avoid use of .bind() in all of our open source projects and in our internal style guide. I don't find the sugar compelling in this case vs. the performance tradeoff. This isn't probably performance critical in this case, but I find it simpler just to avoid using it than to use it only in non-hot code.

[dougwilson]

Hi @nateps, the only place we use .bind is

router/index.js

Line 34 in 6bae01f

: function(fn){ process.nextTick(fn.bind.apply(fn, arguments)) }

which is just a shim for when setImmediate is not in the global. I would recommend just shimming that.

[nateps]

Yes, you are correct, and shimming the global method does solve any concerns about IE 8 compatibility. Just wanted to note the performance consideration since you mentioned the idea of banning .bind().

Personally, I've found this method good to avoid altogether, primarily because it is easier to ask people to avoid things wholesale than to ask for contributors to have good judgement about using certain things only when they are not performance critical.

[dougwilson]

I agree. I avoid .bind in code because of performance reasons with current engines, and I don't find it that much clearer than just stacking functions anyway... :)

[mikermcneil]

@wesleytodd definitely-- although I think @dougwilson should be involved in this too, esp. re http2

Here's the function we use to build the virtual request object in Sails core: https://github.com/balderdashy/sails/blob/master/lib/router/req.js

Here's an example of building the virtual request then squirting it through some middleware (Basically incoming socket.io messages do this):
https://github.com/balderdashy/sails/blob/master/lib/router/index.js#L168

And finally the usage that kicks the whole thing off (btw this is nothing special, and I have no particular attachment to this usage; I'm just linking to it for context-- basically this method ends up triggerring the code that builds the virtual request which I linked to above.): https://github.com/balderdashy/sails/blob/master/test/unit/virtual-request-interpreter.test.js#L12

and the implementation of sails.request(), just to show how it fakes the client streams: https://github.com/balderdashy/sails/blob/master/lib/app/request.js#L27

[mikermcneil]

@wesleytodd Hopefully we can simplify all that ^^ craziness! As is, it works with a lot of middleware out of the box (everything I've tried including the middleware being used there in core). I'm sure there is probably a significant subset of middleware that won't work with it though.

Ideally we get to where we can replace these mocks with some kind of abstractions; i.e. "request emitter" and "response receiver".

[wesleytodd]

@mikermcneil thanks! It will take me a bit to go through that, but the only thing I noticed right off is that you are using express 3 compatible api (I know that is what sails uses). Personally I would rather not do that, and go for express 4/5 support.

@dougwilson Correct me if I am wrong, but the express req/res extend directly from the node classes. Seems like we might not need all of that, especially since I need this in the browser where most of it doesn't make sense to have. Is there a new pillarjs repo for the req/res stuff moving forward? If not maybe we could make one?

Lastly, I was working today on getting my test running with sauce labs like mentioned above. I made some progress, but the modules I was using did not seem to fully work. I have an issue open on one repo and will hopefully be able to get some help, but regardless, progress is being made :)

[felixfbecker]

@wesleytodd @dougwilson How about the router is totally independent of req/res and simply has a dispatch method, and the arguments you pass to dispatch are the exact arguments the router will call middleware with, plus next? So express could just call router.dispatch(req, res) internally and on the browser we could pass our own object, without res? Would that be possible?

[wesleytodd]

@felixfbecker So from my understanding that is was is basically happening now.

https://github.com/wesleytodd/nighthawk/blob/master/lib/router.js#L204
https://github.com/strongloop/express/blob/5.0/lib/application.js#L171
https://github.com/pillarjs/router/blob/master/index.js#L64

Am I wrong? Or rather how is your idea different from those?

[wesleytodd]

I figured out the browser testing stuff. I tried to run the tests here on sauce labs and then realized they are totally not browser compatible. I see two options:

• setup new tests specifically for browsers, might not hit everything, but at least the main stuff
• refactor some of the existing tests to be browser compatible, this would hopefully hit the bulk of the code, at the cost of being a large refactor

The bulk of the the tests are pretty tied into the existing server based setup, which technically isn't necessary because there is nothing specific about matching routes that needs an http server running. Most of the tests could just as easily use mock requests/responses. For example:

    it('should support array of paths', function (done) {
      var cb = after(3, done)
      var router = new Router()
      var server = createServer(router)

      router.all(['/foo', '/bar'], saw)

      request(server)
      .get('/')
      .expect(404, cb)

      request(server)
      .get('/foo')
      .expect(200, 'saw GET /foo', cb)

      request(server)
      .get('/bar')
      .expect(200, 'saw GET /bar', cb)
    })

The above could just as easily be written as (obviously missing some stuff, but just to illustrate the point):

    it('should support array of paths', function (done) {
      var cb = after(3, done)
      var router = new Router()

      router.all(['/foo', '/bar'], saw)

      var req = mockReq('/')
      var res = mockRes()
      router.handle(req, res)
      assert(res.statusCode === 404)

      var req = mockReq('/foo')
      var res = mockRes()
      router.handle(req, res)
      assert(res.statusCode === 200)

      var req = mockReq('/bar')
      var res = mockRes()
      router.handle(req, res)
      assert(res.statusCode === 200)
    })

Which do you all think is a better way to move forward?

[eth0lo]

Hi,

Personally I think it's a great idea, as far I've seen there are a couple of issues to make this happen:

1. req/res abstraction (Moving Request and Response into separate modules)
2. Refactor test to be environment agnostic as @wesleytodd mention
3. Avoid using node internals.
   3.1 Process, can be a simple universal-process-next-tick, which can use the same strategy I mentioned before, this should get rid of the process import, which is huge
   3.2 Time, this is required only by the debug module
   3.3 Buffer, this gets in due the autoheaders feature, which should be solved by 1
4. Set CI for browsers.

Maybe I'm missing something, but this is what i've seen.

I have no problem at all in working on this, in fact I can book one or two full time months to iron this out.

@dougwilson feel free to contact me if you are interested

[wesleytodd]

Hey @eth0lo, Actually most of this work has been done. The title of the issue is "Advertise" support, not make it work. It already fully works and I am using it in multiple production applications. But to hit on your points:

1. This is not the domain of this module, the req/res are passed in. If you are interested in the one that I use see here: https://github.com/wesleytodd/nighthawk/blob/master/lib/request.js https://github.com/wesleytodd/nighthawk/blob/master/lib/response.js
2. Important as mentioned by you and myself above.
3. Are you saying to remove dependence on things that are simply shimmed by browserify and similar tooling? I disagree with this decision. The shims are very small, and everyone I have evert talked to uses a tool to bundle their JS for the front end. All of these tools support shiming that stuff AFAIK.
4. This was the main take away from this ticket that sill needs resolution and probably input from @pillarjs/express-tc

[eth0lo]

The reason why I suggested the aforementioned strategy is because:

Related to 1
I agree is not the domain of the module but still have some features from those that you mention

For one everything that relates to this test, means that trying to bundle router adds buffer.

Also because router parses the url it adds url into the bundle.

Related to 3
When ideally number one happens then the only shim that will be added is process due to that line.

With those changes in place this module could become agnostic of the environment, while the implementors stick to the req and res contracts.

PS. I'm also using this in production already, just wanted to highlight what could be done to be fat free so the clients takes less time to load, as a secondary effect be completely agnostic of the enviroment.

I'm sorry if did not make myself clear before

[dougwilson]

Hi @eth0lo, I'm not sure what you are saying is adding buffer to the build. The "auto head" feature you linked to is implemented with literally the following three lines: https://github.com/pillarjs/router/blob/master/lib/route.js#L103-L105 and I'm not clear how that would cause buffer to get bundled.

[eth0lo]

Let me me double check that

[eth0lo]

Hi @dougwilson, I'm really sorry, I got confused, you are right it's not auto headers it's the auto options when there's no handler associated with a route, in here

For example removing buffer will decrease the bundle size around 38% for a gzip bundle

[wesleytodd]

This is the only line I can find that uses Buffer:

router/index.js

Line 698 in d3a598d

res.setHeader('Content-Length', Buffer.byteLength(allow))

But again, I don't think this is an issue. The bundle is not large to begin with, and all of my apps use buffer for other purposes anyway so it would get included either way for me. I am sure it is pretty common to already use those built-ins if you are bundling modules from npm.

[eth0lo]

@wesleytodd yes that's the only line. Actually when I posted the decreased size, what I did was changing that line for allow.length instead, then bundling as usual. This 'works' because allow will never have a character that is 2bytes in length, personally I think the approximation here is OK, and shave 6Kb (gziped) for those people that do not use buffer in their apps.

In the other hand you are saying that most people use buffer in the builds, actually from my experience is totally the oposite most of the people I know use browserify/webpack/rollup to bundle SPA style apps, which barely use buffer.

BTW, I think this is getting super into details, how about we create a thread for browser support?, and there we discuss file sizes, browser specific stuff, and the migration of the tests, maybe after that we can create a list of stuff get done to properly support browsers as first citizen as node is.

Disclaimer, I continued this thread, because I see more value in contributing to this project than forking it

[wesleytodd]

Digging in a bit more into the functionality here, it is clear that this will never be used in the browser, opened #44 to discuss.

[wesleytodd]

Cleaning up old issues.