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.

[wesleytodd]

I would personally wait until automated test are in place to claim support. An important project like this one cannot just rely on manual testing, it is just not a good practice.

And for the setImmediate issue, I am with @dougwilson on not dropping support. But it seems like @blakeembrey's solution would do just fine. I think we should just tell people they need bind and Array.isArray polyfills for ie9 support.

[nateps]

I hear you on the CI testing, and I'll definitely let you know if I get a chance to work on it.

I think using standard APIs and notifying people that they have to include a polyfill is a very good approach for modern libraries. Less code and simpler to maintain. The only obvious issue is things that can't be polyfilled like __proto__.

I'd vote for simply using setImmediate directly, because it is consistent with the implementation in Express, which @dougwilson wrote as well.

[wesleytodd]

Ill put together the PR to fix the proto issue first thing in the morning. Then at least we can all be happy to use it at our own risk (Edit: with polyfills) until we get CI. Sound alright?

[rlidwka]

I would personally wait until automated test are in place to claim support. An important project like this one cannot just rely on manual testing, it is just not a good practice.

As long as there is no node.js-specific code and native modules, it should work in browser. Manual testing is enough to check this.