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

Compare socket event data using lodash's isEqual instead of indexOf #1061

Merged
merged 4 commits into from Oct 25, 2018

Conversation

rlunden
Copy link
Contributor

@rlunden rlunden commented Oct 17, 2018

When running feathers-sync we had an issue with websockets being triggered with null values when bulk patching:

await context.app.service('foo').patch(
  null,
  {
    text: 'foo'
  },
  {
    query: { // some query }
  }
);

Without feathers-sync this will properly send out one socket event for each affected row in the foo service. However with feathers-sync active the sockets would instead be sent out with null as their data instead of the actual object.

See issue previously reported and closes feathersjs-ecosystem/feathers-sync#96

Upon further investigation it seems the issue is not technically in feathers-sync but in transport-commons. The issue seems to be that getDispatcher uses indexOf when trying to get individual item to dispatch. This normally works fine because the check that indexOf performs under the hood will check the reference and without feathers-sync the references will always match. However, the way feathers-sync works is that it will JSON.stringify the object and then JSON.parse the object thus creating a new reference. So indexOf will then return false and no data will be found for the socket to emit.

We fixed this by using lodash's isEqual with .find to find the correct individual item to dispatch. In our limited testing this seems to fix the issue when running feathers-sync while not breaking anything when running without feathers-sync.

Using indexOf will return false when comparing identical objects with different references. This in turn breaks feathers-sync since it creates a new object using JSON.parse, meaning that data won't properly sent out when for example patching multiple rows at once.
@daffl
Copy link
Member

daffl commented Oct 25, 2018

Good catch, thank you! Will be published shortly.

@daffl daffl merged commit f706db3 into feathersjs:master Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk patching a service from within another service returns null values
2 participants