fix: Use WeakMap to connect socket to connection (#1509)

feathersjs · Aug 17, 2019 · 64807e3 · 64807e3
1 parent 39cc8e0
commit 64807e3
Show file tree

Hide file tree

Showing 11 changed files with 46 additions and 55 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/authentication-oauth/src/express.ts b/packages/authentication-oauth/src/express.ts
@@ -33,21 +33,22 @@ export default (options: OauthSetupSettings) => {
 
     authApp.get('/:name', (req, res) => {
       const { feathers_token, ...query } = req.query;
+      const { name } = req.params as any;
 
       if (feathers_token) {
         debug(`Got feathers_token query parameter to link accounts`, feathers_token);
         req.session.accessToken = feathers_token;
       }
 
-      res.redirect(`${path}/connect/${req.params.name}?${qs.stringify(query)}`);
+      res.redirect(`${path}/connect/${name}?${qs.stringify(query)}`);
     });
 
     authApp.get('/:name/callback', (req: any, res: any) => {
       res.redirect(`${path}/connect/${req.params.name}/callback?${qs.stringify(req.query)}`);
     });
 
     authApp.get('/:name/authenticate', async (req, res, next) => {
-      const { name } = req.params;
+      const { name } = req.params as any;
       const { accessToken, grant } = req.session;
       const service = app.defaultAuthentication(authService);
       const [ strategy ] = service.getStrategies(name) as OAuthStrategy[];

diff --git a/packages/primus/lib/index.js b/packages/primus/lib/index.js
@@ -6,12 +6,13 @@ const http = require('http');
 const Emitter = require('primus-emitter');
 
 const debug = makeDebug('@feathersjs/primus');
-const socketKey = Symbol('@feathersjs/primus/socket');
 
 function configurePrimus (config, configurer) {
-  return function () {
-    const app = this;
+  return function (app) {
+    // Returns the connection object
     const getParams = spark => spark.request.feathers;
+    // Mapping from connection back to its socket
+    const socketMap = new WeakMap();
 
     if (!app.version || app.version < '3.0.0') {
       throw new Error('@feathersjs/primus is not compatible with this version of Feathers. Use the latest at @feathersjs/feathers.');
@@ -58,12 +59,7 @@ function configurePrimus (config, configurer) {
               next();
             }, 0);
 
-            primus.on('connection', spark =>
-              Object.defineProperty(getParams(spark), socketKey, {
-                value: spark
-              })
-            );
-
+            primus.on('connection', spark => socketMap.set(getParams(spark), spark));
             primus.on('disconnection', spark => app.emit('disconnect', getParams(spark)));
           }
 
@@ -81,13 +77,12 @@ function configurePrimus (config, configurer) {
 
     app.configure(commons({
       done,
-      socketKey,
+      socketMap,
       getParams,
       emit: 'send'
     }));
   };
 }
 
 module.exports = configurePrimus;
-module.exports.SOCKET_KEY = socketKey;
 module.exports.default = configurePrimus;
diff --git a/packages/primus/test/index.test.js b/packages/primus/test/index.test.js
@@ -63,8 +63,7 @@ describe('@feathersjs/primus', () => {
     options.server.close(done);
   });
 
-  it('exports default and SOCKET_KEY', () => {
-    assert.ok(primus.SOCKET_KEY);
+  it('exports default', () => {
     assert.strictEqual(primus, primus.default);
   });
 

diff --git a/packages/socketio/index.d.ts b/packages/socketio/index.d.ts
@@ -8,6 +8,5 @@ interface FeathersSocketIO {
   (callback?: (io: io.Server) => void): () => void;
   (options: number | io.ServerOptions, callback?: (io: io.Server) => void): () => void;
   (port: number, options?: io.ServerOptions, callback?: (io: io.Server) => void): () => void;
-  readonly SOCKET_KEY: unique symbol;
   default: FeathersSocketIO;
 }
diff --git a/packages/socketio/lib/index.js b/packages/socketio/lib/index.js
@@ -4,8 +4,6 @@ const http = require('http');
 const { socket: commons } = require('@feathersjs/transport-commons');
 const debug = require('debug')('@feathersjs/socketio');
 
-const socketKey = Symbol('@feathersjs/socketio/socket');
-
 function configureSocketio (port, options, config) {
   if (typeof port !== 'number') {
     config = options;
@@ -18,9 +16,11 @@ function configureSocketio (port, options, config) {
     options = {};
   }
 
-  return function () {
-    const app = this;
+  return function (app) {
+    // Function that gets the connection
     const getParams = socket => socket.feathers;
+    // A mapping from connection to socket instance
+    const socketMap = new WeakMap();
 
     if (!app.version || app.version < '3.0.0') {
       throw new Error('@feathersjs/socketio is not compatible with this version of Feathers. Use the latest at @feathersjs/feathers.');
@@ -50,16 +50,12 @@ function configureSocketio (port, options, config) {
               .listen(port || server, options);
 
             io.use((socket, next) => {
-              const connection = {
+              socket.feathers = {
                 provider: 'socketio',
                 headers: socket.handshake.headers
               };
 
-              Object.defineProperty(connection, socketKey, {
-                value: socket
-              });
-
-              socket.feathers = connection;
+              socketMap.set(socket.feathers, socket);
 
               next();
             });
@@ -89,7 +85,7 @@ function configureSocketio (port, options, config) {
 
     app.configure(commons({
       done,
-      socketKey,
+      socketMap,
       getParams,
       emit: 'emit'
     }));
@@ -98,4 +94,3 @@ function configureSocketio (port, options, config) {
 
 module.exports = configureSocketio;
 module.exports.default = configureSocketio;
-module.exports.SOCKET_KEY = socketKey;
diff --git a/packages/socketio/test/index.test.js b/packages/socketio/test/index.test.js
@@ -77,8 +77,7 @@ describe.only('@feathersjs/socketio', () => {
     server.close(done);
   });
 
-  it('exports default and SOCKET_KEY', () => {
-    assert.ok(socketio.SOCKET_KEY);
+  it('exports default', () => {
     assert.strictEqual(socketio, socketio.default);
   });
 

diff --git a/packages/transport-commons/src/socket/index.ts b/packages/transport-commons/src/socket/index.ts
@@ -10,16 +10,16 @@ const debug = Debug('@feathersjs/transport-commons');
 export interface SocketOptions {
   done: Promise<any>;
   emit: string;
-  socketKey: any;
+  socketMap: WeakMap<RealTimeConnection, any>;
   getParams: (socket: any) => RealTimeConnection;
 }
 
-export function socket ({ done, emit, socketKey, getParams }: SocketOptions) {
+export function socket ({ done, emit, socketMap, getParams }: SocketOptions) {
   return (app: Application) => {
     app.configure(channels());
     app.configure(routing());
 
-    app.on('publish', getDispatcher(emit, socketKey));
+    app.on('publish', getDispatcher(emit, socketMap));
     app.on('disconnect', connection => {
       const { channels } = app;
 

diff --git a/packages/transport-commons/src/socket/utils.ts b/packages/transport-commons/src/socket/utils.ts
@@ -35,14 +35,13 @@ export function normalizeError (e: any) {
   return result;
 }
 
-export function getDispatcher (emit: string, socketKey: any) {
+export function getDispatcher (emit: string, socketMap: WeakMap<RealTimeConnection, any>) {
   return function (event: string, channel: CombinedChannel, context: HookContext, data: any) {
     debug(`Dispatching '${event}' to ${channel.length} connections`);
 
     channel.connections.forEach(connection => {
-      // The reference between connection and socket
-      // is set in `app.setup`
-      const socket = connection[socketKey];
+      // The reference between connection and socket is set in `app.setup`
+      const socket = socketMap.get(connection);
 
       if (socket) {
         const eventName = `${context.path || ''} ${event}`.trim();

diff --git a/packages/transport-commons/test/socket/index.test.ts b/packages/transport-commons/test/socket/index.test.ts
@@ -17,7 +17,7 @@ describe('@feathersjs/transport-commons', () => {
     options = {
       emit: 'emit',
       done: Promise.resolve(provider),
-      socketKey: 'test',
+      socketMap: new WeakMap(),
       getParams () {
         return connection;
       }

diff --git a/packages/transport-commons/test/socket/utils.test.ts b/packages/transport-commons/test/socket/utils.test.ts
@@ -9,6 +9,7 @@ import {
   getDispatcher,
   runMethod
 } from '../../src/socket/utils';
+import { RealTimeConnection } from '../../src/channels/channel/base';
 
 describe('socket commons utils', () => {
   describe('.normalizeError', () => {
@@ -59,27 +60,30 @@ describe('socket commons utils', () => {
 
   describe('.getDispatcher', () => {
     it('returns a dispatcher function', () =>
-      assert.strictEqual(typeof getDispatcher('test', 'here'), 'function')
+      assert.strictEqual(typeof getDispatcher('test', new WeakMap()), 'function')
     );
 
     describe('dispatcher logic', () => {
       let dispatcher: any;
       let dummySocket: EventEmitter;
       let dummyHook: any;
       let dummyChannel: any;
+      let dummyConnection: RealTimeConnection;
+      let dummyMap: WeakMap<any, any>;
 
       beforeEach(() => {
-        dispatcher = getDispatcher('emit', 'test');
+        dummyConnection = {};
+        dummyMap =  new WeakMap();
+        dispatcher = getDispatcher('emit', dummyMap);
         dummySocket = new EventEmitter();
         dummyHook = { result: 'hi' };
         dummyChannel = {
-          connections: [{
-            test: dummySocket
-          }],
+          connections: [ dummyConnection ],
           dataFor (): null {
             return null;
           }
         };
+        dummyMap.set(dummyConnection, dummySocket);
       });
 
       it('dispatches a basic event', done => {