Unregistering services and hooks

[Download]

Currently, once the app is running, it's pretty difficult to unregister Feathers services and hooks.

We would like to be able to support unregistering all event handlers associated with a route, so we can recreate and re-register them while the server keeps running. If we do this right, it will enable people to use hot reloading techniques such as Webpack's Hot Module Replacement with their Feathers services and hooks.

API suggested by @daffl in Slack:

app.service('/myservice',  null)

would completely undo the effects of

app.use('/myservice', myservice)

...and thus allow us to use HMR like this:

services/myservice/index.js

'use strict';

let app; // hang on to the app object
module.exports = function(){
  app = this;
  register()
};

function register() {
  // re-require on every invocation to facilitate HMR
  // `require` caches modules once loaded, so second require is fast.
  const hooks = require('./hooks');
  const Service = require('./service');
  app.use('/myservice', new Service());
  const service = app.service('/myservice');
  service.before(hooks.before);
  service.after(hooks.after);
}

function unregister() {
  app.service('/myservice', null);
}

if (module.hot) {
  module.hot.accept(['./hooks', './service'], function(){
    unregister(); // unregister the old service and hooks
    register(); // ..and register the new ones. 
  })
}

// not sure if it's useful to keep this next line...  modules referencing 
// `Service` would need to make sure they get the new one somehow...
// module.exports.Service = Service;

[ElegantSudo]

That's a bit complicated. Is there a way we can augment this, so that on each deploy these tasks are already being reviewed and executed?

[Download]

@ElegantSudo Can you elaborate on what is complicated? Basically the contents from the example above come from:

1. the feathers CLI generator (but moved the logic into a function register)
2. the Webpack HMR api (e.g. see the example on their Wiki)
3. The unregister function, which is basically the essence of this proposal

I think most of this code would be generated for you by feathers generate service.

[ElegantSudo]

@Download that's a valid point. What I really meant is: can we add this to feathers so that this process is automated upon server deploy for every application?

I think hot module reloading is an essential feature for any developer with an application.

[marshallswain]

The generator will be pluggable, so customization like this will be possible. We probably don't want a Webpack build in the default generator, but adding it through a generator plugin should be fairly straightforward.

[Download]

@marshallswain @daffl I cannot set the milestone for this issue, but it should be set to 'Buzzard' according to the conversations in Slack.

[Download]

I'm having doubts about the currently proposed API..

Let's suppose we had done this:

app.use('/myservice', myService)

Then we could unregister myService by calling app.use again with null as the 2nd argument:

app.use('/myservice', null)

Ok... But does this imply that we could also have done:

app.use('/myservice', someOtherService)

In other words, does app.use always unbind any old service that was bound to that URL, or does it only happen when we explicitly pass null ?

If it always unbinds the old service and passing null (or undefined?) is just a way to say that there is no new service to replace the old one than I think that's great. If however in the second scenario when we app.use('/myservice', someOtherService) it doesn't unbind the old service, so null is a magic value to trigger the unregister, then I think it would be better to just make a separate method... unuse for example.

Any thoughts?

@marshallswain @daffl @ekryski

[marshallswain]

We can make it so that

app.service('my-service', null)          // tears down an existing service. 
app.service('my-service', newService())  // tears down and replaces.

For teardown, you would have to pass null, otherwise it too closely matches app.service('my-service'). Passingnull gives it a sense of explicitness.

[ekryski]

or we move to something like app.mount('/users', userService()) and app.unmount('/users'). If you want to reference a service you still simply do app.service('users') and in order to support backwards compatibility we can alias app.service('/users', userService()) to app.mount and can toss a deprecation warning.

That might also make it more clear on the client that you are just referencing a server side service most of the time. I feel like that is something that people get a bit confused about because you use app.service to both register and reference a service where on the client you aren't registering many services. Thoughts?

[marshallswain]

Yeah. I like separating the API like that.

app.mount('users', userService())   //setup or replace
app.unmount('users')                //teardown
app.service('users')                //lookup

[Download]

Agreed. And since this issue is planned for Buzzard it would be good timing for such an API change.

I like how Feathers builds upon Express, but I do get quitte confused at which methods are from Express and which from Feathers sometimes. In fact I think I messed up the terms already in my comments above. Having the API separated like this will make it clearer I think.

[daffl]

In the jQuery days it used to be pretty common to use methods as getters and setters at the same time (e.g. $().attr('href') and $().attr('href', 'value') but I guess it has fallen a little out of fashion.

I'm not sure if introducing new methods is necessary though. When we implement #258 and make Feathers framework independent it will have its own API and not extend Express anymore. The only downside I see in that case to stick with app.use('/service', myService) I can see is that it looks to Express-y and might be mistaken as a way to register middleware functions (which it won't anymore). Like @marshallswain said, it could teardown implicitly (and be able to set it to null) and we make app.service('name') always return the service and drop support for app.service('name', myService).

[Download]

I think this discussion is relevant:
app.use('/location', service) vs app.service('/location', service) #287

• Come Buzzard, Express is going to be just another server for Feathers, so use is becoming less clear
• use is already overloaded pretty heavily by Express itself. Even in Express I get confused by it...
  • For request/error handlers
  • For middleware
  • For subapps
• service is unique to Feathers afaik, and is pretty descriptive

app.service('users', userService())   // setup (replaces any old service)
app.service('users', null)            // teardown
app.service('users')                  // lookup

This looks pretty clear to me. I know I have, as a new user, spent some time reading the Feathers JS source code because I didn't understand what extra stuff Feathers' use was doing that Express' use does not. Having it be a completely separate method will make it much clearer that it does do extra special things.

I looked into what it takes to perform unregistering a service but I got a headache from it :) Express itself does not make it easy, because unregister is something that has to be hacked in by accessing 'private' state in the app object if I can believe the Stack Overflow anwsers on questions relating to it.

[daffl]

I agree, I'd prefer that syntax, too. If everybody is on board sticking with this we might even want to consider updating all the service examples from .use to .service in preparation for #258.

I'm always surprised that Express never added support for unregistering middleware (if you have a reference to the middleware function it shouldn't be too hard to splice it out of the stack). I guess Express hot module reloading is just hacking it the same way then. Unregistering the socket events will definitely be more straightforward.

[Download]

I agree, I'd prefer that syntax, too. If everybody is on board sticking with this we might even want to consider updating all the service examples from .use to .service in preparation for #258.

Yeah it would be good to start out by clearly defining what the API should look like. I think service is the best candidate. We could keep the use override in place but warn that it will be deprecated.

About Express... How I'm personally dealing with it (and I've found I can do this for some Feathers JS stuff as well) is that I'm placing the HMR code in a wrapper handler that then takes care of hot-reloading the real handler when needed, like this:

var server = require("http").createServer()
server.on("request", wrapperHandler)
server.listen(8080)

function wrapperHandler(req, res, next) {
  // re-require every request to facilitate HMR
  // This does not pose a performance issue in production, as `require` caches loaded modules
  var requestHandler = require("./handler.js")
  return requestHandler.call(this, req, res, next)  
}

// check if HMR is enabled
if (module.hot) {
    // accept update of dependency
    module.hot.accept("./handler.js", function() {
        // no need to do anything here as module will automatically be re-required on next request
       console.info('Hot-reloaded handler.js')
    })
}

We could consider following the same strategy in Feathers.... The problem is that you can only replace code this way, but not really remove it. It works very well for HMR but is not applicable to other scenarios.

But Express does not really support unregistering... There is only removeListener from the http package but I'm not sure if it covers all bases. It sure is possible to make it work with Express, but the question is whether Feathers should try to support something that Express itself does not support. I think it would be great for Feathers to support it but it's important to realize it will be a maintenance effort. Looking back at Express the code needed to do it has changed over the major releases so it's not unlikely it will happen again.

EDIT: Forgot to mention it but +1 on updating the docs. I'd be willing to pitch in with that.

[ekryski]

Ok that's fine by me. I would prefer to have an explicit unregister or unmount function as opposed to having to know to set null on a service. We have a few things like that in our code base where it is not clear what the functionality is unless you read the docs and then we end up fielding a bunch of questions about them.

Whenever we can I'd like to move away from esoteric commands to being more explicit (I'm also guilty of this).

I'm down with this:

app.service('users', userService())   // setup (replaces any old service)
app.service('users', null)            // teardown, any falsy value should be ok
app.service('users')                  // lookup

But would prefer this:

app.mount('users', mw1, userService(), mw2, mw3)   // setup (replaces any old service), alias to app.service('users', userService())
app.unmount('users')               // teardown, any falsy value should be ok, alis to app.service('users', null)
app.service('users')                  // lookup
app.register(middleware)         // registering middleware, -> alias for app.use
app.unregister(middleware)     // is this valuable?

This provides a bit more explicit syntax regarding the action being taken and something we can promote going forward but would still allow apps that haven't migrated to use the functionality.

---

This is also part of a much bigger discussion regarding #258. What should the common feathers API look like? From what I'm thinking about on a grander scale the app.service('users', null) might be the better syntax.

I'm going to brain dump over on #258.

[ElegantSudo]

app.service('users', userService())   // setup or replace a service
app.service('users', null)            // teardown/remove a service
app.service('users')                  // lookup and return service

I think this is a really elegant way of doing it. It makes a lot of sense to me.

Whereas mount vs. unmount, service, and register vs. unregister might be making it more complicated than it has to be.

[kethan]

Will this work now ? if i use app.service('users', null) ??

[daffl]

No, this has not been implement yet.

[kethan]

remove(app, path) {
        this.app._router.stack.forEach((layer) => {
            if (!layer) return;
            if (layer && !layer.match(path)) return;
            if (['query', 'expressInit'].indexOf(layer.name) != -1) return;
            if (layer.path == path) {
                let idx = this.app._router.stack.indexOf(layer);
                this.app._router.stack.splice(idx, 1);
            }
        })
    }
remove(app, '/users');

I have implemented removing service like this...I don't know whether it is the safe/correct way to do??

[daffl]

Yes. That will work for REST endpoints but not with websockets (Primus, Socket.io). The problem is that we currently can't easily get a reference to the socket listeners we set up to unbind them.

[PavelPolyakov]

hi @daffl,

just want to clarify.

I've the react app with hot-module-reload, where I have a component (some debug code is already there):

import * as React from "react";
import {autobind} from "core-decorators";
import {connect} from "react-redux";
import {app} from "../feathers";
console.log(app.listeners());

window.app = app;

import {Button} from 'reactstrap';

@connect(store => ({}))
@autobind
export default class Test extends React.Component {
    constructor(props) {
        super(props);

        this.state = {
            plannings: []
        };
    }

    componentWillMount() {
    }

    componentDidMount() {
        const a =app.service('/plannings').on('created', (planning) => {
            console.log('planning was created', planning);
            this.setState((prevState, props) => {
                prevState.plannings.push(planning);
                return prevState;
            });
        });

        console.log('a', a);
    }

    newPlanning() {
        app.service('/plannings').create({'hello': 'hello'});
        console.log('done, planning created');
    }

    render() {
        return (
            <div>
                <div>plannings: {JSON.stringify(this.state.plannings.length)}</div>
                <ul>
                    {_.map(this.state.plannings, (planning, index) => {
                        return <li key={index}>{JSON.stringify(planning)}</li>
                    })}
                </ul>

                <Button onClick={this.newPlanning}>new planning</Button>
            </div>
        );
    }
}

whenever I update something in the feathersjs app, the hot module reload thing is in action, everything updates. however, with the next newPlanning function call, the on('created' event is fired N times. And, as I understand, for the cases of the first subscriptions, I see the next warning:

Is this happens because of the topic of this issue?
I also try to add there something like:

//Hot Module Replacement API
if (module.hot) {
    module.hot.accept('../feathers', () => {
        console.log('very hot');
        console.log(app);
        app.service('/plannings').removeListener('created',() => {});
    });
}

it seems like working when I update ../feathers app, but fails when I update the component itself. I use socketio for the client.

Regards,

[PavelPolyakov]

@daffl
is there a way to receive all the subscriptions for the service on demand?
tried to investigate the service object, but was not able to found that list.

if they are stated somewhere - it should be possible to unsubscribe on module hot reload.

[PavelPolyakov]

At the end I think I found the solution.
Here is the thing which I add to the bottom of the component:

//Hot Module Replacement API
if (module.hot) {
    module.hot.dispose(() => {
        app.service('/plannings').removeAllListeners('created');
    });
}

Here are the docs about the .dispose, I think the docs for the webpack 2 are not yet updated.

[noahprince22]

Any updates on this? Will this be implemented?

My use case is that I want to replace services with updated versions of themselves when database tables change. Basically, I'm auto generating routes based on a database table.

My current workaround is this:

/*
Feathers/express doesn't let you unregister services. See https://github.com/feathersjs/feathers/issues/446

So, if you want to be able to replace services with other services, you need to keep a local reference to your
most recent instance of the service at a given path. Then, use the DelegatorService to delegate to the most
recent instance.
 */
class DelegatorService {
  constructor (options) {
    this.options = options || {};
    this.serviceName = options.serviceName;
    this.serviceRegistrar = options.serviceRegistrar;
  }

  get service() {
    return this.serviceRegistrar.getService(this.serviceName);
  }

  find (params) {
    return this.service.find(params);
  }

  get (id, params) {
    return this.service.get(params);
  }

  create (data, params) {
    return this.service.create(data, params);
  }

  update (id, data, params) {
    return this.service.update(id, data, params);
  }

  patch (id, data, params) {
    return this.service.patch(id, data, params);
  }

  remove (id, params) {
    return this.service.remove(id, data, params);
  }

  setup(app) {
    return this.service.setup(app);
  }
}

class ServiceRegistrar {
  constructor() {
    this.services = {};
  }

  use(app, serviceName, service) {
    const existing = this.services[serviceName];
    // Always overwrite service
    this.services[serviceName] = service;

    if (!existing) {
      app.use(serviceName, new DelegatorService({
        serviceName,
        serviceRegistrar: this
      }));
    }
  }

  getService(serviceName) {
    return this.services[serviceName];
  }
}

export default ServiceRegistrar;

[daffl]

Given that Express does not have an official way to remove middleware and removing Socket listeners will add a whole bunch of additional complexity probably not, at least not soon.

There usually is a better way around this (e.g. in hooks by just skipping them conditionally) or disabling services via hooks.

[arleighdickerson]

https://gist.github.com/arleighdickerson/c16cf00a2dd96b39c2c3a5fb2fb211f3

[matt-d-rat]

Given that this issue was raised back in 2016 and the framework has gone through a number of improvements since then, I was wondering what the status of this issue is?

In the interest of avoiding further bike-shedding, do we think we have enough of a consensus on an approach to add these features in order to support HMR? I have HMR setup for my applications, and it works some of the time, with the exception of updating services.

[lock]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.

[dyedwiper]

I'm not totally sure if this is fitting here: I just had a problem in a test where I needed to unregister a service and I solved it with deleting the key in app.services: delete app.services['path/to/your/service']