search

feathersjs-ecosystem / feathers-reactive

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

Fix integration tests #194

Closed daffl closed 2 years ago

daffl commented 2 years ago

This pull request is a follow up to the updated integration tests by @claustres in https://github.com/feathersjs-ecosystem/feathers-reactive/pull/192

The problem wasn't in feathers-reactive but in the tests. When creating messages on the server or on another client, the observable never got triggered because the tests finished before any of the events propagated to the client instance. The solution was to wait for the first return value on the client and then run the tests.

claustres commented 2 years ago

Good catch thanks :+1:

daffl commented 2 years ago

Everything should be working as expected then right? You mentioned you had issues with the new version.

claustres commented 2 years ago

Unfortunately not yet @daffl. I've dug deeper and it seems to me there is something related to service path and heading slash. Typically we are using an API prefix in our app so that all services are registered on path like /api/myservice. What I can see here is that a callback is typically registered on event name like this /api/myservice created, but when the event is raised here the name is like this api/myservice created so that it does not match any registered callback, which explains why no listeners are called.

It seems I can fix this on our side by removing the heading slash on service path when registering it but I have to perform more testing as this might also lead to regressions. Maybe something has changed about this in v4 ou v5, any insight is welcome.

daffl commented 2 years ago

If that is the problem then it might a regression. I ran some quick tests and double checked the code and it did look like the slashes are remove everywhere so a breaking example would be helpful.