Closed wesleytodd closed 7 months ago
@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 :)
@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?
@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?
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:
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?
Hi,
Personally I think it's a great idea, as far I've seen there are a couple of issues to make this happen:
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
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:
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
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.
Let me me double check that
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
This is the only line I can find that uses Buffer
: https://github.com/pillarjs/router/blob/d3a598d3afec5bf55d683f25887bbbf8fd65e4f8/index.js#L698
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.
@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
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.
Cleaning up old issues.
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).