feathersjs-ecosystem / feathers-reactive

Reactive API extensions for Feathers services
MIT License
216 stars 37 forks source link

Cannot call addListener on the client service connection #190

Closed claustres closed 2 years ago

claustres commented 2 years ago

Steps to reproduce

Listen to events like this:

app.service('myservice').watch({ listStrategy: 'smart' })
        .find({ query })
        .subscribe(response => { ... })

Expected behavior

No error is raised, the same code worked fine with Feathers v3.

Actual behavior

The following error is raised with either Feathers v4 or v5: image

I've tried to dig into what is going on but I do not really understand the code fully. As far as I understand feathers services implement this mixin. The transport abstraction expects that the underlying connection also implements a similar interface. But as far as I can see, the connection object, which is a Socket for socket.io, implements a slightly different interface. For instance if I debug our app, I can see that the connection object has a addEventListener method but not a addListener method as expected. Could it be linked to a change in interface within socket.io ?

System configuration

NodeJS version 16, Webpack 5, Feathers v4 or v5

"@feathersjs/client": "^5.0.0-pre.22"
"feathers-reactive": "^0.9.0"
"socket.io-client": "^4.4.1"
RobMaple commented 2 years ago

I've just run into the same issue -- reverting to feathers-reactive 0.8.2 has stopped the issue. I'm assuming the issue is relating to a dependecy update in 0.9.0

daffl commented 2 years ago

I added an integration test for this in https://github.com/feathersjs-ecosystem/feathers-reactive/pull/191/files#diff-6bb01a9664f0d2819bc4e2290b685dddc6dfb10fd786e7fa30933ae5ec57212b and it is currently passing. So either this issue will be fixed with merging and publishing #191 or there is something else that needs to be done to reproduce.

claustres commented 2 years ago

Just to let you know that I've tested my app with the PR https://github.com/feathersjs-ecosystem/feathers-reactive/pull/191 but the error is still here :-( Could you just clarify the expected behavior about the event emitter interfaces according to my first comment @daffl ? Indeed, my understanding is that it is related to isomorphic code and that we don't have the right interface on the client-side vs server-side. Inspecting deeper we can see that on the server-side socket.io is using standard NodeJS event emitter interface (https://github.com/socketio/socket.io/blob/main/lib/typed-events.ts#L1) but on the client side this is another matter. At some point they switched from https://github.com/component/emitter to their own https://github.com/socketio/emitter, and the interface is only "partially" compatible with the server-side one. This could be the explanation to the fact the integration test on the server-side is not catching the problem.

My main question remains: how could it have worked previously as this isomorphic code in Feathers was already the same in v3 ? Maybe we should be less strict about the interface and do not throw an error if a method is missing ? We should also add all possible methods like addListener.

Not sure if this can be related: https://github.com/socketio/socket.io-client/issues/1461.

daffl commented 2 years ago

When you set up a Socket.io client (app.configure(socketio(socket))) the client service just passes the emitter methods to the socket. If the method doesn't exist on socket it throws that error (https://github.com/feathersjs/feathers/blob/dove/packages/transport-commons/src/client.ts#L26).

So it is likely related to the Socket.io client version combined with what RxJS is trying to do with fromEvent. What I don't understand is why the test that I linked - which uses the latest Socket.io on the server and the client - appears to be working. It would be good to be able to reproduce the problem in that test.

RobMaple commented 2 years ago

I've managed to reproduce the issue in a new bare bones app based off the code used for the test. The only real difference is that I added Express to serve the server side app. One thing to note is that the error comes after the client has successfully recieved data -- not sure if this is causing the test to miss the following error?

daffl commented 2 years ago

@RobMaple Yep, that was totally it. I am seeing the same error now. It appears that a Socket.io socket still isn't Node Event Emitter compatible and does not have the addListener method. The following fixes it:

    const socket = io('http://localhost:3030');
    socket.addListener = socket.addEventListener;
    const client = feathers()
      .configure(socketioClient(socket))
      .configure(rx({ idField: 'id' }));

This should probably just be aliased in the client so that the service is fully event emitter compatible.

daffl commented 2 years ago

https://github.com/feathersjs/feathers/pull/2686 has the proper fix for this. Basically RxJS fromEvent wants an "event target" which the Socket.io connection does support but the client service did not pass through. The fix mentioned above should still work for previous versions.

claustres commented 2 years ago

From a design perspective I would say that the fix should probably be in the feathers-reactive mixin if possible. Indeed, it seems to me a little weird to add this in feathers client because using the client does not mean that we will use the reactive module and it's always risky to change object interface under-the-hood if not required.

daffl commented 2 years ago

Well, the methods do exist on the socket object so I think it makes sense to add them to the service as well. A direct fix has been published in feathers-reactive@0.10.0 now as well.

claustres commented 2 years ago

God bless you @daffl.

claustres commented 2 years ago

I have upgraded an app with v0.10, the error has gone, however none of my subscribed listeners is called whenever a service event is raised. I can see the event in the websocket messages but nothing seems to happen. Maybe this can be related to some changes in v5, I will try to assess what is going on. @daffl Have you tested the react example with this new release ? Thanks for feedback

daffl commented 2 years ago

I have not but it should be possible to update the integration test at https://github.com/feathersjs-ecosystem/feathers-reactive/blob/master/test/integration.test.js to fail pretty easily in this case.

claustres commented 2 years ago

You are right, I did not have a look to these tests. So I started https://github.com/feathersjs-ecosystem/feathers-reactive/pull/192 and the behavior seems to me a little strange but maybe my tests are not right.

It seems that the reactive listener is correctly called when the client initiates the service operations itself but not when they are initiated server-side. It seems to be almost the case when initiated by another client except the created event that is missed in the test. Any insight is welcome @daffl.

Kelvince01 commented 1 year ago

The addListener is deprecated, use as below with typescript , (this._socket as any).addListener = this._socket.addEventListener;