Fix for Issue #331 #943
Fix for Issue #331 #943
Conversation
…ary namespaces.
Fix for Issue socketio#331
…ingFix * commit '5fe7373610ef92de0d34b44d503c22cab5712c8d': Release 1.4.5 package: bump `engine.io-client`
sync to master branch
rebuild client
…ify flow and handle complex event flow.
pass query as socket property.
This reverts commit 74aff26.
handles passing of query via query strings add tests for functionality
Complete functionality. Add Test.
remove console statement.
|
We don't have style guide, but please follow existing styles like the following:
It would be nice to introduce |
| var socket = io('/abc', {query: {a:'b'}}); //passes in as a query obj | ||
| var socket2 = io('/abcd?b=c&d=e'); //passes in as a query string | ||
| expect(socket.query).to.be('a=b'); | ||
| expect(socket2.query).to.be('b=c&d=e'); |
There was a problem hiding this comment.
It'd be nice if we could test if query parameter was actually sent to server?
There was a problem hiding this comment.
The corresponding PR on socket.io has the test that checks for the query parameter being sent over. Do you think that's ok?
There was a problem hiding this comment.
Then it would be ok, thanks!
zweihan
force-pushed
the
queryStringFix
branch
2 times, most recently
from
324b5ef
to
433366e
Compare
previous commit [here](3407f3f).
zweihan
force-pushed
the
queryStringFix
branch
from
433366e
to
f01a349
Compare
| @@ -401,6 +401,7 @@ Manager.prototype.destroy = function(socket){ | |||
| Manager.prototype.packet = function(packet){ | |||
| debug('writing packet %j', packet); | |||
| var self = this; | |||
| if (packet.query && packet.type == 0) packet.nsp += '?' + packet.query; | |||
There was a problem hiding this comment.
I wonder this packet can be { nsp: '/nsp', data:query }, so that we don't need to construct and parse query string.
There was a problem hiding this comment.
I think query can be in data rather than appending to nsp. Any thoughts about that ? cc @rauchg
There was a problem hiding this comment.
That's possible, but that would require changes to Socket.io but the corresponding PR has already been merged.
|
Thanks! Can you fix the conflict ? |
* orgmaster: (44 commits) bump zuul Remove jspm browser config Run lint before test instead of before build ESlint manual fix Eslint autofix Add env to eslintrc Exclude generated files from linting updated babel-eslint dep to 4.1.7 (4.1.6 broken) refactor gulpfile Add eslint to gulpfile and create gulp task default fixed regex Rename gulp task webpack to build Remove commented make recipe removed babel react preset Makefile use gulp from node_modules instead of global Remove browserify stuff, add zuul-builder-webpack devDep Migrate zuul config from yml to js Refactor webpack config out to support/webpack.config.js removed ide-specific entries from gitignore updated makefile to use gulp commands ... Conflicts: lib/manager.js lib/socket.js
Query string fix
|
@nkzawa Updated and fixed the conflict. Tests passed locally, including linting. |
|
Thank you! |
Fixes #331.
Send query string over together with CONNECT packets.
Query associated with each socket is stored as a property of that socket.