-
Notifications
You must be signed in to change notification settings - Fork 10.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Loading client script on demand. #2567
Conversation
Support for the serving of the client script mess with packagers like browserify, webpack. Especcialy in projects where it is not used at all. This patch is workaround to avoid that problem in the cases when client script is not served.
@@ -94,6 +94,11 @@ Server.prototype.checkRequest = function(req, fn) { | |||
Server.prototype.serveClient = function(v){ | |||
if (!arguments.length) return this._serveClient; | |||
this._serveClient = v; | |||
|
|||
if(v && !clientSource) { | |||
clientSource = read(require.resolve('socket.io-client/socket.io.js'), 'utf-8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make more sense to load the client source when a client actually requests the client source (in the Server's .serve() method)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it will add small delay for read() and require.resolve() on first request. As it is written now is more close to the original code so such side effects are avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @palavrov, side effects are much easier avoided using this method, and for what it's worth I can't see it being an improvement to do it otherwise.
Please add a space between the if
and the following (
though, and also indent with two spaces not four, just to be picky 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologise for not explaining myself
I didn't mean to just move those lines to the the Server's .serve() method.
I was implying to do something similar to pillarjs/send (the streaming static file server that express's serve-static is built on).
An example for this would look like:
// In the top of index.js
var fs = require('fs');
var clientPath;
Server.prototype.serve = function(req, res){
var etag = req.headers['if-none-match'];
if (etag) {
// Use cached version.
}
res.setHeader('Content-Type', 'application/javascript; charset=utf-8');
res.setHeader('ETag', clientVersion);
res.writeHead(200);
clientPath = clientPath || require.resolve('socket.io-client/socket.io.js');
var readStream = fs.createReadStream(clientPath);
readStream.pipe(res);
};
Serving static files this way has multiple benefits:
- The file doesn't unnecessary consume memory if it is not requested.
- Instead of buffering the entire file into memory, it is asynchronously streamed in small chunks (usually 64kb) to the response.
- The file's chunks can be garbage collected once they were served (which can save memory if it is not accessed frequently).
- We avoid unnecessary converting the
UTF-8
binary into a string and vise-versa when the client source is flushed to the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it will be better. For me the only reason to mess with this was that require.resolve() is called always but is not emulated by webpack/browserify i.e. it is breaking my application for something that I don't use at all. To be honest I don't see any benefit to serve the client here - may be it is handy for one scenario but not for others. So my goal was to make it more optional and used just on demand without messing too much.
@palavrov Do you mind if I ask why are you trying to bundle the socket.io server? If you're browserify/webpack app is using socket.io you can just require |
@benjamin-albert Actually the bundling is a side effect of using nexe to prepare a standalone web app that can be easily installed i.e. end users should install this app in their intranet and then use web interface build on top of feathersjs which uses socketio for bi directional communication with the app. Everything works very well with just few small glitches like this one. Even on windows ... the final version should be installed as windows service with nice setup etc. Edit: For the frond end I'm also using socket.io client with webpack and ot works as expected - there is no drama there. |
@palavrov thanks! |
Support for the serving of the client script mess with packagers like browserify, webpack. Especcialy in projects where it is not used at all. This patch is workaround to avoid that problem in the cases when client script is not served.
Support for the serving of the client script mess with packagers like
browserify, webpack. Especcialy in projects where it is not used at all.
This patch is workaround to avoid that problem in the cases when client
script is not served.