feat(docs): migration guide for remoting hooks by bajtos · Pull Request #4426 · loopbackio/loopback-next

bajtos · 2020-01-14T13:57:47Z

Resolve #3950:

Write content for docs/site/migration/models/remoting-hooks.md, explain how to migrate LB3 remoting hooks to LB4 interceptors.

Explain how to map properties provided by LB3 remoting context to LB4 concepts
and how to access that data via Dependency Injection.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

agnes512

Nice start with a detailed content 👍 . Some comment on vague parts.

docs/site/migration/models/remoting-hooks.md

bajtos · 2020-01-16T15:53:35Z

@agnes512 thank you for thoughtful comments. I have slightly reworked the overall structure of this page and renamed few section titles. Is the high-level picture more clear now?

@hacksparrow thank you for the review too, do you have any more comments?

I pushed a new commit 6b27dad that improves the older text and adds the remaining pieces. I consider this page as done and ready for final review(s).

@strongloop/loopback-maintainers PTAL.

docs/site/migration/models/remoting-hooks.md

jannyHou

👍 Very detailed and well organized contents!

Reviewed the sections before " Modifying request parameters", LGTM so far. Will take a look of the rest and try the get current user tomorrow.

docs/site/migration/models/remoting-hooks.md

agnes512

👏 I quickly scan through and it looks good to me. I like the structure especially the part that how users can modify their LB3 artifacts to corresponding LB4 methods/classes. Just some nitpicks!

agnes512 · 2020-01-17T14:09:42Z

docs/site/migration/models/remoting-hooks.md

+
+In order to reject a request and return an error HTTP response, just throw an
+error from your interceptor. As explained in
+[Handling errors in controllers](../../Controllers.html#handling-errors-in-controllers),


I don't know if Controllers.html should be Controllers.md?

Good catch! I was copying full URLs from https://loopback.io and changing them to local paths. I have to review all links and replace .html to .md where appropriate.

Thank you for catching this problem! ❤️

agnes512 · 2020-01-17T14:20:50Z

docs/site/migration/models/remoting-hooks.md

+are registered for models.
+
+In LoopBack 4, the REST API is implemented by controllers that are decoupled
+from models, therefore interceptor are registered and invoked on controller


interceptor -> interceptors

agnes512 · 2020-01-17T14:28:55Z

docs/site/migration/models/remoting-hooks.md

 ## Accessing context data

-(to be done)
+In LoopBack 3, a remoting hooks receives a context object providing both


This sentence is a bit long.
Suggestion:
In LoopBack 3, a remoting hook receives a context object providing both transport-specific data, such as the HTTP request & response objects, and transport-independent data, such as the array of input arguments for the remote method and the result of remote method invocation.

I am going to rework this paragraph into a list.

In LoopBack 3, a remoting hook receives a context object providing: - Transport-specific data like the HTTP request & response objects. - Transport-independent data, for example the array of input arguments for the remote method and the result of remote method invocation.

nabdelgadir

LGTM 👏

nabdelgadir · 2020-01-17T14:34:39Z

docs/site/migration/models/remoting-hooks.md

+
+{% include note.html content=" In LoopBack 3, a model class has three
+responsibilities: a model describing shape of data, a repository providing
+data-access APIs, and a controller implementing REST API). Both remoting hooks


Nitpick: closing bracket but no opening one

nabdelgadir · 2020-01-17T14:35:12Z

docs/site/migration/models/remoting-hooks.md

+
+In LoopBack 4, the REST API is implemented by controllers that are decoupled
+from models, therefore interceptor are registered and invoked on controller
+classes/method, not on a model classes/methods. " %}


method -> methods

nabdelgadir · 2020-01-17T14:41:08Z

docs/site/migration/models/remoting-hooks.md

+method is invoked.
+
+{% include warning.html content="
+If your interceptor is registered for more than a single method, then extra care is needed to ensure your interceptor is making valid assumption about the arguments expected by the target controller method.


Nitpick: assumption -> assumptions

nabdelgadir · 2020-01-17T14:42:35Z

docs/site/migration/models/remoting-hooks.md

+
+## Accessing the current user
+
+LoopBack 4 does not provide authentication layer out of the box, it relies on


Nitpick: , -> ;

nabdelgadir · 2020-01-17T14:43:14Z

docs/site/migration/models/remoting-hooks.md

+about the current user.
+
+If you are using the official
+[LoopBack Authentication Extension](../../lb4/Loopback-component-authentication.html),


../../lb4/Loopback-component-authentication.html -> ../../Loopback-component-authentication.md

dhmlau

I have very limited knowledge about remoting hooks in LB3, but your PR is clear to me. LGTM.
I have one suggestion to consider, please see my review comment. Thanks!

dhmlau · 2020-01-17T14:53:51Z

docs/site/migration/models/remoting-hooks.md

+) {
+  try {
+    // Add pre-invocation logic here
+    const result = await next();


Just a suggestion.. Referencing the next paragraph, would it be helpful to mention as a comment here something like:

// add code from beforeRemote hook here

If that's the case, we can also flip the order of the 2 paragraphs too.

This code snippet is showing the the interceptor implementation as created by lb4 interceptor, I'd like to keep it as-is.

I like the idea of making it more explicit where to put before/after remote hooks, I'll update the code snippet in the example below.

jannyHou

👏 LGTM

emonddr

Great write-up, Miroslav!

bajtos · 2020-01-20T10:41:09Z

Pushed to commits to address the remaining review feedback:
0f6eea3 and b68386e. I'm going to clean up the commit history now and proceed towards landing this pull request.

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

bajtos added Docs Migration labels Jan 14, 2020

bajtos added this to the Jan 2020 milestone Jan 14, 2020

bajtos self-assigned this Jan 14, 2020

bajtos force-pushed the docs/migrate-models-remoting-hooks branch from eac3ef2 to afa2744 Compare January 14, 2020 13:59

bajtos requested a review from a team January 14, 2020 13:59

agnes512 reviewed Jan 14, 2020

View reviewed changes

hacksparrow reviewed Jan 16, 2020

View reviewed changes

docs/site/migration/models/remoting-hooks.md Outdated Show resolved Hide resolved

bajtos commented Jan 16, 2020

View reviewed changes

docs/site/migration/models/remoting-hooks.md Show resolved Hide resolved

bajtos marked this pull request as ready for review January 16, 2020 15:57

bajtos requested a review from raymondfeng as a code owner January 16, 2020 15:57

jannyHou reviewed Jan 17, 2020

View reviewed changes

docs/site/migration/models/remoting-hooks.md Outdated Show resolved Hide resolved

bajtos requested a review from agnes512 January 17, 2020 08:23

agnes512 approved these changes Jan 17, 2020

View reviewed changes

nabdelgadir approved these changes Jan 17, 2020

View reviewed changes

dhmlau approved these changes Jan 17, 2020

View reviewed changes

jannyHou approved these changes Jan 17, 2020

View reviewed changes

emonddr approved these changes Jan 17, 2020

View reviewed changes

bajtos force-pushed the docs/migrate-models-remoting-hooks branch from 49f5e8c to 36fbdae Compare January 20, 2020 09:42

feat(docs): migration guide for remoting hooks

0da51b1

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

bajtos force-pushed the docs/migrate-models-remoting-hooks branch from b68386e to 0da51b1 Compare January 20, 2020 10:41

bajtos merged commit 52cd1df into master Jan 20, 2020

bajtos deleted the docs/migrate-models-remoting-hooks branch January 20, 2020 12:13


		## Accessing the current user

		LoopBack 4 does not provide authentication layer out of the box, it relies on

Conversation

bajtos commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

agnes512 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bajtos commented Jan 16, 2020

Uh oh!

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agnes512 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabdelgadir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhmlau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

emonddr left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jan 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bajtos commented Jan 14, 2020 •

edited

Loading