Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework current-context docs #227

Merged
merged 1 commit into from
Jan 11, 2017
Merged

Rework current-context docs #227

merged 1 commit into from
Jan 11, 2017

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Jan 4, 2017

Rework the documentation page "Using current context" - drop mentions of the old loopback-context module, describe the new official solution implemented in strongloop/loopback#1495

Connect to strongloop/loopback#1495

@crandmck @superkhau please review

@bajtos bajtos self-assigned this Jan 4, 2017
@bajtos bajtos added the #review label Jan 4, 2017
@bajtos bajtos changed the title [WIP] Rework current-context docs Rework current-context docs Jan 5, 2017
};
```

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line

@crandmck
Copy link
Contributor

crandmck commented Jan 6, 2017

I have a quite a few minor wording suggestions. In general, some of the text reads more like a blog than documentation (in docs, we usually avoid using first person). In general, my comments are the suggested revision to the text, except for italicized text which is my meta-comments/questions :-)

@bajtos If you don't have time to go through and make all these changes, I'd be glad to make the ones of which you approve (directly in your branch before merging the PR). In some cases, I have questions though which need to be addressed.

LoopBack applications sometimes need to access context information to implement the business logic, for example to:

* Access the currently logged-in user.
* Access the HTTP request (such as URL and headers).

A typical request to invoke a LoopBack model method travels through multiple layers with chains of asynchronous callbacks. It's not always possible to pass all the information through method parameters. 

**See also**: [Example in LoopBack repository](https://github.com/strongloop/loopback/blob/master/example/context/app.js).
In LoopBack 2.x, we have introduced current-context APIs which used the module
Copy link
Contributor

Choose a reason for hiding this comment

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

LoopBack 2.x introduced current-context APIs using the continuation-local-storage module ...

problems (for example, see [issue #59](https://github.com/othiym23/node-continuation-local-storage/issues/59)).
As a result, our current-context feature does not work in many situations,
as can be seen from issues reported in LoopBack's
[issue tracker](https://github.com/strongloop/loopback/issues?utf8=%E2%9C%93&q=is%3Aissue%20getCurrentContext).

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this module is not reliable and has many known problems (for example, see issue #59). As a result, the current-context feature does not work in many situations, (see related issues).

standalone module [loopback-context](https://github.com/strongloop/loopback-context)
and removed all current-context APIs in the version 3.0 (see
[Release Notes](3.0-Release-Notes.html#current-context-api-and-middleware-removed) for
more details).

Copy link
Contributor

Choose a reason for hiding this comment

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

To address this problem, LoopBack 3.0 moves all current-context-related code to the loopback-context module and removed all current-context APIs (see
Release Notes).

[Operation Hooks](Operation-hooks.html). Until there is a reliable
implementation of continuation-local-storage available for Node.js, we are
recommending to explicitly pass any additional context via `options` parameter
of (remote) methods.
Copy link
Contributor

Choose a reason for hiding this comment

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

However, applications clearly need to acces information like the currently logged-in user in application logic, for example in operation hooks. Until there is a reliable implementation of continuation-local-storage available for Node.js, explicitly pass any additional context in the options parameter of (remote) methods.

at Layer.handle [as handle_request] (.../node_modules/express/lib/router/layer.js:95:5)
...
```
All built-in methods like
Copy link
Contributor

Choose a reason for hiding this comment

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

In LoopBack 3.0, all built-in methods ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Built-in method were modified in both 3.x and 2.x. Should I explicitly mention this fact?

(IIRC, "options" parameter was added before we started this current-context rework.)

Copy link
Contributor

@crandmck crandmck Jan 10, 2017

Choose a reason for hiding this comment

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

If it applies to both v2&3, then perhaps word as follows:

Built-in methods such as PersistedModel.find or PersistedModel.create accept an options argument.

Once you've enabled context propagation, you can access the current context object using `LoopBackContext.getCurrentContext()`.
The context will be available in middleware (if it is loaded after the context middleware), remoting hooks, model hooks, and custom methods.
When strong-remoting is resolving the "options" argument, it will call model's
method `createOptionsFromRemotingContext`. The default implementation of this
Copy link
Contributor

Choose a reason for hiding this comment

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

When strong-remoting resolves the "options" argument, it calls model's createOptionsFromRemotingContext method.


```javascript
var LoopBackContext = require('loopback-context');
There are several ways how to customize this value.
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several ways to customize this value:

  • Override createOptionsFromRemotingContext in your model.
  • Use a "beforeRemote" hook.
  • Use a custom strong-remoting phase.


Because the "options" parameter is a regular method parameter, it can be
accessed from remote hooks via `ctx.args.options`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the "options" parameter is a regular method parameter, you can access it rrom remote hooks via ctx.args.options.

...
Again, a hook like this can be reused by placing the code in a mixin.

Note that remote hooks are executed in order controller by the framework, which
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing--what "framework"? LoopBack? Is the order indeterminate?

Note that remote hooks are executed in order controlled by the framework, ...
Note typo: controlled is what you meant, not "controller", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll improve this sentence a bit, but let's discuss more in the "main" thread.

### Use a custom strong-remoting phase

Internally, strong-remoting uses phases similar to [Middleware
Phases](https://loopback.io/doc/en/lb3/Defining-middleware.html). The framework
Copy link
Contributor

Choose a reason for hiding this comment

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

@cgole cgole removed the #review label Jan 7, 2017
@bajtos
Copy link
Member Author

bajtos commented Jan 9, 2017

@crandmck thank you for the review. I have fixed (almost) all your comments in e28628f, but there are few of them that deserve further discussion, see below.

#227 (comment)

In LoopBack 3.0, all built-in methods

Built-in method were modified in both 3.x and 2.x. Should I explicitly mention this fact?

(IIRC, "options" parameter was added before we started this current-context rework.)

#227 (comment)

This is confusing--what "framework"? LoopBack? Is the order indeterminate?

Note that remote hooks are executed in order controlled by the framework, ...
Note typo: controlled is what you meant, not "controller", right?

The order is determinate AFAIK, but it may be difficult to setup the application is such way that one gets exactly the order they want.

I don't know all the details, but it looks like the hooks are invoked from most-specific (e.g. MyModel.myMethod) to least specific (e.g. MyModel.* and then **), at least according to https://gist.github.com/ebarault/fdc39c5d07cc40f08664256f9e00905a#file-context-js-L242-L248:

here, we use Model.beforeRemote hook instead of originally used RemoteObject.before hook
since we want remoteContext injection to perform before any other Model.beforeRemote hook
(Model.beforeRemote hooks are called before RemoteObject.before hooks)
NOTE: the remotingContext will be available in the Model.beforeRemote('**', function(ctx, instance, next) {...}) hook
(/!\ only the ** one), provided that you are able to register it after the one trigger in the iteration below
you can still discriminate the remote method name in that hook using the ctx.mehod.name property
(see: strongloop/strong-remoting#340)

I think (but didn't verify), that if there are multiple hooks registered at the same level of specificness, then the order will be controlled by the order in which these hooks were registered. This order is difficult to control, because it's basically controlled by loopback-boot and how it invokes different kinds of scripts (model .js file, mixins, components, boot scripts).

So for example, if write a boot script that registers a beforeRemote hook adding a new options property, and you also register a beforeRemote hook in your model .js file (e.g. common/models/my-model.js) where you would like to read this added property, then you are out of luck (at least I believe so).

As far as this document goes, I don't want to dig into specific details, just advise the user that the order in which hooks are invoked may be out of their control and if the order is important for them, then they should use a different solution.

How can I express this information better than I do it now?

@crandmck
Copy link
Contributor

Built-in method were modified in both 3.x and 2.x. Should I explicitly mention this fact?

If it applies to both vers, then I suggest this:

Built-in methods such as PersistedModel.find or PersistedModel.create accept an options argument.

As far as this document goes, I don't want to dig into specific details, just advise the user that the order in which hooks are invoked may be out of their control and if the order is important for them, then they should use a different solution.

How about this:

It may not always be possible to control the order in which remote hooks are executed. If you need to control the order, then use use a custom strong-remoting phase as described in the following section.

@bajtos
Copy link
Member Author

bajtos commented Jan 11, 2017

@crandmck thank you, I applied your suggestions in 3994cae.

I am going to rebase & squash the commits and land the pull request now.

@bajtos bajtos merged commit f252614 into gh-pages Jan 11, 2017
@bajtos bajtos deleted the rework-context-propagation branch January 11, 2017 16:07
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.

4 participants