Move reactive functions to service(...).watch(), use rxjs5

[j2L4e]

Includes the following changes

breaking changes

• service methods now remain unchanged (only return Promises)
• services now have a .watch() method returning an object with service methods which return Observables.
• constructor signature changed from function(Rx, [options]) to function([options])
• rxjs5 is a hard dependency now. Only used methods are imported, so any additional operators need to be imported from the rxjs/add namespace.
• options.idField is now a mandatory option

non-breaking

• added a simple caching mechanism on .find and .get methods leveraging the rxjs shareReplay operator. Only prevents multiple simultaneous subscriptions. When the subscription count drops to zero, the cache is cleared.
• options.let takes a function that can alter the created Observables (is passed to .let)
• rx options can be passed to .watch() now, i.e. service.watch({ strategy: 'never' }).find() equals service.watch().find({ rx: { strategy: 'never' } })
• removed obsolete options (lazy, rx = false)
• removed obsolete promisification code and tests
• changed existing tests to match new API, added tests for options.let and caching functionalities
• added proper typings
• refactored and cleaned up code
• removed babel dependency according to feathersjs/feathers#608

accomplishments

• Service methods are not overloaded anymore and can now be typed properly (TypeScript).
• Reduced bundle sizes (rxjs imports)
• Observables can be lazy while fire-and-forget functionality is maintained
• Promisification of Observables could lead to unintended side effects with functions that take a Promise or an Observable as an argument (e.g. angular's async pipe). This is no longer the case.
• Relying on idField's default settings could break 'smart' strategy with databases that don't use id by default.
• The caching minimizes remote service calls and minimizes caching boilerplate

[daffl]

I like it once we make sure that all the tests are passing. I'd still like to investigate if we could implement #43 because it seems like the cleanest way to deal with it to me. Maybe add a .lazy() to not introduce the regression you mentioned there?

[j2L4e]

Tests fixed.

[j2L4e]

Re: #43
I don't see the point here (maybe out of ignorance, though).
As far as I understand Symbol.observable aims to provide an interface to create Observables from non-Observable resources. However, we internally use OBservables already, so there's nothing left to interface.
Implementing TC39 would only make sense if there was a use-case for feathers-reactive without Observables. Yet its entire purpose is to bring Observables to feathers services.
To implement Symbol.observable (so that it makes sense) the entire module would need to be rewritten in an event-based manner, so that you could use it without rxjs. However stuff like the smart strategy is just the perfect use-case for rxjs.

[daffl]

I thought it's just meant for implementing your own "Observablification"? So a normal service call even with using feathers-reactive would still return an actual promise but something like

const ob = Observable.from(app.service('messages').find());

ob.subscribe(page => console.log('Messages page is', page);

Would return the Observable that you currently get from calling .find directly.

[j2L4e]

I took a look at rxjs5's source: https://github.com/ReactiveX/rxjs/blob/master/src/observable/FromObservable.ts

Observable.from(app.service('messages').find()) would in fact create a FromObservable rather than a PromiseObservable if [Symbol_observable] is present on a Promise. So the returned Observable wouldn't necessarily mimic Promise behavior (i.e. be hot, emit once, then complete). However, the Promise would have already been created, so the Observable would either need to be hot itself or call the underlying logic once again.

Imho TC39 is rather thought to provide an interface for non-trivial and rather custom Objects where the to-be-created Observable's behavior can't be infered from the type.

Creating an observable from a feathers service's hook-chain for example. (Whatever that would look like)

[psi-4ward]

PS: Don't forget to document how to add methods to Observable :)

[j2L4e]

going to close and clean up

[daffl]

One thing I was wondering is if we should call it .rx() instead of .watch() and pass the feathers-reactive options to it (.rx(options)) instead of params.rx. Thoughts?

[j2L4e]

I like passing the options in there. Params is heavily overloaded already, so i think anything that can be moved somewhere else should be. I'd prefer .watch() though, as it's more declarative and less technical. But that's personal preference. Am 12. Juli 2017 16:37:42 MESZ schrieb David Luecke <notifications@github.com>:

…

One thing I was wondering is if we should call it `.rx()` instead of `.watch()` and pass the `feathers-reactive` options to it (`.rx(options)`) instead of `params.rx`. Thoughts? -- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: #58 (comment)

-- Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

[j2L4e]

This is what I came up with:

    const paramsPositions = {
      update: 2,
      patch: 2
    };

    const mixin = {
      cache: cache,

      rx(options = {}) {
        this._rx = options;
        return this;
      },
      watch(options = {}) {
        const boundMethods = {};

        Object.keys(reactiveMethods).forEach(method => {
          const position = typeof paramsPositions[method] !== 'undefined' ? paramsPositions[method] : 1;

          boundMethods[method] = (...args) => {
            args[position] = Object.assign(args[position] || {}, {rx: options});
            return reactiveMethods[method](...args);
          };
        });

        return boundMethods;
      }
    };

It simply moves the argument of watch() into params.rx so everything else can remain as-is.
I find this far from pretty, though...

[j2L4e]

I'm just reopening this, because I didn't get around to recommitting the changes in a clean way. I'm aware that reviewing changes like this sucks and I'm sorry for that. A review would still be much appreciated.

There shouldn't be too much stuff in there that's controversial functionality-wise anyway.

I did some refactoring to make parts of the code more readable and understandable, also moved some stuff to utils.js.

If you need parts of the commit mess to be reverted, I'm fine with doing that. I'd just really like to see this merged.

In case you still want .watch() renamed to .rx() the old functionality of .rx() would need to move to .rx().persistOptions() or similar.

Updated the first post with a complete "what's new" list.

[patrickpereira]

This looks interesting! Hopefully can be merged soon 👍

[patrickpereira]

@j2L4e How can I use your master already now, I've tried with git://github.com/j2L4e/feathers-reactive.git#master in my package.json, but keep getting this error when importing:

ERROR in ./src/app/api.service.ts
Module not found: Error: Can't resolve 'feathers-reactive' in '/Users/patrickpereira/portal/src/app'
resolve 'feathers-reactive' in '/Users/patrickpereira/portal/src/app'
  Parsed request is a module
  using description file: /Users/patrickpereira/portal/package.json (relative path: ./src/app)
    Field 'browser' doesn't contain a valid alias configuration
  after using description file: /Users/patrickpereira/portal/package.json (relative path: ./src/app)
    resolve as module

[j2L4e]

just npm installing from #master works, I think you're having a webpack configuration issue here

[patrickpereira]

@daffl When will this be merged with the official repo?

[daffl]

Thank you again for doing this @j2L4e, sorry I took so long. Released as v0.5.0, let's see how it goes 😄

[daffl]

@j2L4e I also reverted some of the .gitignore changes because we're trying to not include infrastructure changes in feature branches. If you find something is missing in .gitignore you can submit a PR against https://github.com/feathersjs/generator-feathers-plugin/ and we'll make sure it'll be updated in all repositories.

[daffl]

Also, can we add a note on how to upgrade to 0.5 to the readme?

[j2L4e]

I see, sorry for that one.
I think I'll put together a couple lines on how to... well your last comment just came in ;)