-
Notifications
You must be signed in to change notification settings - Fork 10.1k
Serve slim client #2776
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
Serve slim client #2776
Conversation
Makes lib/index.js stream the client files gzipped
Adds a test for source map serving
res.writeHead(200); | ||
res.end(clientSourceMap); | ||
var readStream = createReadStream(path.join(clientRoot, file)); | ||
if (~req.headers['accept-encoding'].indexOf('gzip')) { |
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'm not sure this if is necessary.
I don't think we support any browser that doesn't support gzip.
Hi, thanks for the PR! A question though: shouldn't the gzip work rather be handled by some reverse proxy? |
@darrachequesne Thank you for bringing up this question! To be quire honest the main reason I opened this pull is so I can update examples with: <script src='/scoket.io/socket.io.slim.js'></script> However as far as I can tell, examples are the only reason Socket.IO should be dealing with the (pretty big) burden of serving static files. When I use Socket.IO I:
In fact the chat example is the only time I ever used this feature. However I understand that all of these options are a nightmare for documentation because:
Bottom lineSetting up a reverse proxy can sometimes be more complicated (and might involve corporate politics) compare to one of the options I suggested above. You never used this feature because you care about micro optimizations (removing the close to nothing burden of gzipping the client). You use it because it already does everything for you, and gzipping should be one of those things especially with the growing number of users on mobile. |
If gzipping is stopping the PR from being merged we can move that to a separate pull request so I can update the docs with the new slim client. |
@benjamin-albert actually, I would like to fix the issues related to minified/map files before merging your PR: |
@darrachequesne I also need to load the slim version of socket.io.js using something like:
because (long-story short) loading socket.io.js (with its debug code) into a web page controlled by Electron.js can trigger a full 4 second delay during compilation ... if more than one instance of the Electron-based app is running simultaneously. Can you please advise about how to proceed? A new PR? |
Superseded by 7603da7 (included in |
The kind of change this PR does introduce
A code change that improves performance.
Current behaviour
The slim client is not served.
New behaviour
The new slim client (and its source map) are served.
Other information (e.g. related issues)
This refactor also:
I also added test for serving the source maps and slim client.