Skip to content

Add app.ready() and Model ready hooks - #532

Closed
ritch wants to merge 1 commit into
masterfrom
feature/ready
Closed

Add app.ready() and Model ready hooks#532
ritch wants to merge 1 commit into
masterfrom
feature/ready

Conversation

@ritch

@ritch ritch commented Sep 4, 2014

Copy link
Copy Markdown
Member

@bajtos @raymondfeng @fabien @kraman

This adds a set of ready hooks to Model classes. Override these hooks to implement custom behavior that depends on your app being bootstrapped:

  • models attached to datasources, methods mixed in
  • relations, remoting, and other metadata defined
  • boot scripts run, loopback-boot complete
  • etc.

Docs

Do not call this method if you are using loopback-boot to bootstrap your application.
loopback-boot will call this method for you! Otherwise you must call app.ready() to run
the Model.ready() hooks.

Calling ready() will call ready() on all models attached to the app. Override
the ready() method on a Model class to ensure you have access to a bootstrapped application.

module.exports = function(MyModel) {
  MyModel.setup = function() {
    // setup is called when a model is extended
    // you must call `base.setup()` to extend the base class properly
    this.base.setup();

    // add or remove remote methods and otherwise modify the `Model`
    this.remoteMethod('myMethod');
  }

  MyModel.beforeReady = function(app, cb) {
    // async setup, runs after all models are `setup()`
    // and before `ready()` is called
    setTimeout(cb, 100);
  }

  MyModel.ready = function(app) {
    // MyModel and other classes can be used
    // you should not modify any classes in this method
    console.log(this.sharedClass.methods());
  }
}

@ritch
ritch force-pushed the feature/ready branch 2 times, most recently from 857cad8 to ff6652c Compare September 4, 2014 01:45
@ritch

ritch commented Sep 4, 2014

Copy link
Copy Markdown
Member Author

@bajtos where in loopback-boot would it make sense to call ready()?

@ritch

ritch commented Sep 4, 2014

Copy link
Copy Markdown
Member Author

It might also make sense to add DataSource#ready() and similar hooks. By default these could wait for an actual connection and that sort of thing.

Model.beforeReady() could call this.dataSource.connect()...

@clark0x

clark0x commented Sep 4, 2014

Copy link
Copy Markdown
Contributor

@ritch does Model has a ready event?

@ritch

ritch commented Sep 4, 2014

Copy link
Copy Markdown
Member Author

@ritch does Model has a ready event?

Nope

@kraman

kraman commented Sep 4, 2014

Copy link
Copy Markdown
Contributor

@ritch Can you list a few examples of what is possible in the setup, beforeReady and ready phases. For example, defining or hiding remote methods and modifying the model goes in setup. How about beforeReady ?

@clark0x

clark0x commented Sep 4, 2014

Copy link
Copy Markdown
Contributor

@ritch The loopback.rest() and API Explorer middlewares are mounted during boot, so if app.ready() is called after loopback-boot complete, they will have been mounted already, changing sharedMethods in Model.ready() hooks will be too late.

@bajtos

bajtos commented Sep 4, 2014

Copy link
Copy Markdown
Member

@ritch

// setup is called when a model is extended
// you must call `base.setup()` to extend the base class properly
this.base.setup();

This is a very bad example:

  • setup method accepts parameters, you are discarding them
  • If you have A extends B extends C, and call B.setup on A, this.base is B, not C.
  • In the same A-B-C example, B.setup is called with this pointing to B, not A.

This is the corrected version:

MyModel.setup = function() {
    // setup is called when a model is extended
    // you must call `base.setup()` to extend the base class properly
    MyModel.base.setup.apply(this, arguments);

    // etc.
 }

@bajtos

bajtos commented Sep 4, 2014

Copy link
Copy Markdown
Member

@ritch where in loopback-boot would it make sense to call ready()?

See lib/executor. Since you want ready to be called after boot scripts are loaded, call it after runBootScripts (lib/executor.js#27).

Comment thread lib/application.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix this per my comment above.

@bajtos

bajtos commented Sep 4, 2014

Copy link
Copy Markdown
Member

I am seconding @clarkorz @kraman and would like to know what are the use cases for these new hooks? We should be careful to not introduce a feature that would be confusing to loopback users and/or make it easy to use it incorrectly (e.g. set up remoting too late).

Another thing to consider is inconsistency between beforeReady and ready - why is the former an async hook, while the latter is sync? I would rather have both of them async (or perhaps sync, depending on the use cases you have in mind).

@clark0x

clark0x commented Sep 4, 2014

Copy link
Copy Markdown
Contributor

@bajtos I want to hide some of the shared methods. So I'm seeking an event/hook/etc. to do such thing. (FYI, Currently I set some flags in model's JSON file and create a boot script 0-remoting.js to read model.settings and do the job )

related issue:

related PR:

The PR works in following conditions:

  1. the shared method that we want to hide is exist
  2. loopback.rest() and API Explorer haven't been mounted

Currently, loopback emits attached and dataSourceAttached event for model. But at these moments, relations and scopes might haven't been created. strong-remoting won't find shared methods for pending relations and scopes.

So I thought, I need an event or hook that a model has been fully configured. But after discuss with @raymondfeng in that PR, I realize that this is a false proposition, because we could add relations and scopes even after loopback bootstrapped (maybe remove and alter in the future?). That's why I propose an configChanged event emit on model by juggler.

@bajtos bajtos added the #review label Sep 6, 2014
@ritch ritch self-assigned this Sep 9, 2014
@ritch ritch mentioned this pull request Sep 9, 2014
@ritch ritch added #sprint54 and removed #review labels Sep 9, 2014
@ritch

ritch commented Sep 11, 2014

Copy link
Copy Markdown
Member Author

I think ready is where the confusion is coming from... I think defined or frozen might be less confusing. Essentially it would be an event and or hook that ensures a given model (or the entire app) has been defined.

The main use case for this is for the explorer, angular sdk, and anything that wants reads metadata defined at runtime. Eg. a script that lists all available remote methods...

function listRemoteMethods(app) {
  app.models().forEach(function(model) {
    model.sharedClass.methods().forEach(function(sharedMethod) {
      console.log(sharedMethod.name);
    });
  });
}

The issue we keep running into is... when can I run listRemoteMethods() and be certain that all methods will be available? Consider an app that does this...

setTimeout(function() {
  app.models.MyModel.remoteMethod(...);
}, ...);

This method will not be listed, and it won't be clear why. Sure the example is contrived, but there is nothing in loopback that makes it clear that you cannot do this. Adding a defined phase / event / hook would allow us to do things like throw after a class has been defined in Model.remoteMethod() and other functions that modify a Model.

Here's the fix to the issue mentioned above with the proposed defined event.

MyModel.define = function(cb) {
  setTimeout(function() {
    MyModel.remoteMethod(...);
    cb();
  }, 1);
}

app.on('defined', function() { listRemoteMethods() });

@kraman

kraman commented Sep 12, 2014

Copy link
Copy Markdown
Contributor

Ran into this exact issue when working on adding relations to the remote connector yesterday.

How about being a bit more prescriptive about the lifecycle stages. Something like the stages below (you will need to define the actual stages since I am not very familiar with loopback model initialization).

  1. Event after all DS are "connected" and available so models can start attaching
  2. Event after all models are "attached" so relations can be defined
  3. Event after all relations have been defined and models are "defined" so remote methods etc. can be listed and relation/other scopes etc. can be created.
  4. Event after all scopes have been created so that models are "frozen" and can no longer be modified

@raymondfeng

Copy link
Copy Markdown
Member

@kraman I agree. We need to define the lifecycle phases and corresponding hooks/events so that developers can understand when to do what.

@bajtos

bajtos commented Sep 12, 2014

Copy link
Copy Markdown
Member

when can I run listRemoteMethods() and be certain that all methods will be available?

In some cases, the correct answer is "never". Models can be added and modified even after the application started, there was at least one user asking for that feature.

Therefore the event name "frozen" is misleading.

From the POV of strong-remoting and swagger/explorer, a better solution is to provide change events at the level of remoting metadata, i.e. method was added, removed, reconfigured.

From the POV of angular code generator, it still makes sense to have an app-wide event fired when the models available at boot time are fully configured. (Related: strongloop/grunt-loopback-sdk-angular#5)

@altsang altsang removed the #sprint54 label Sep 23, 2014
@ritch

ritch commented Feb 6, 2015

Copy link
Copy Markdown
Member Author

This was a dead end... Closing for now.

@ritch ritch closed this Feb 6, 2015
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.

6 participants