Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update socket entity #521

Merged
merged 5 commits into from Jun 8, 2017

Conversation

Projects
None yet
3 participants
@marshallswain
Copy link
Member

marshallswain commented Jun 1, 2017

Fixes: #293

This updates the socket entity for either primus or socket.io. (it would need a small refactor to work for both)

@ekryski
Copy link
Member

ekryski left a comment

@marshallswain @daffl I'd love to hear what you guys think about the socket listener comment I made. Other than that, this is totally awesome and has been a long time coming! Much ❤️ to @marshallswain!

module.exports = function updateEntity (entity, meta) {
const { app } = meta;
const authConfig = app.get('auth');
var idField = app.service(authConfig.service).id;

This comment has been minimized.

@ekryski

ekryski Jun 2, 2017

Member

super knit pick so not a big deal. Use let instead of var? Same on line 14.

@@ -139,5 +141,8 @@ export default function setupSocketHandler (app, options, { feathersParams, prov
socket.on('authenticate', authenticate);
socket.on(disconnect, logout);
socket.on('logout', logout);

entityService.on('updated', updateEntity);

This comment has been minimized.

@ekryski

ekryski Jun 2, 2017

Member

The only thing I'm wondering about is whether we should do

app.io.on(`${entity} updated`, updateEntity)
app.io.on(`${entity} patched`, updateEntity)

so that filters don't get applied. Since this is an internal listener we probably don't want any filters that the developer may have registered to apply here. Otherwise they might be inadvertently preventing an update.

However, there is also a trade-off with that. If we don't use the app.service() method then we can't support listening to any remote decoupled services. In practice that might not be all that realistic and my idea of being able to make auth a standalone service separate from the entity might not actually be possible. Usually a users service and auth service live on the same app instance.

marshallswain added some commits Jun 2, 2017

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Jun 8, 2017

Since filters don't run on the server, but operate at the provider level, I'm going to go ahead and merge this in.

@marshallswain marshallswain merged commit 6944eca into master Jun 8, 2017

4 checks passed

codeclimate no new or fixed issues
Details
codeclimate/coverage 84.58% (+0.1%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@marshallswain marshallswain deleted the update-socket-entity branch Jun 8, 2017

@jerfowler

This comment has been minimized.

Copy link
Contributor

jerfowler commented Jun 21, 2017

Noticed the comparison doesn't work when the entityId is an ObjectID (mongodb/mongoose) :

if (entityId === socketEntityId) {
    Object.assign(socketEntity, entity);
}

Plus, using Object.assign() over the top of socketEntity doesn't remove properties if they were deleted from entity.

Also, getting errors when there are open unauthenticated connections, which I opened a new ticket here: #529

Wondering if it would be better to do so something like this:

Object.keys(socketMap).forEach(socketId => {
    const socket = socketMap[socketId];
    const feathers = socket.feathers || socket.request.feathers
    const socketEntity = feathers && feathers[authConfig.entity];

    if (socketEntity) {
      const socketEntityId = socketEntity[idField];

      if (`${entityId}` === `${socketEntityId}`) {
        feathers[authConfig.entity] = entity;
      }
    }
  });
@jerfowler

This comment has been minimized.

Copy link
Contributor

jerfowler commented Jun 21, 2017

I created a pull request with some test cases, here: #531

Similar to above with the exception I had to keep the Object.assign(). Looks like you need to assign to the existing entity object otherwise it breaks external references. So now I just delete the properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.